Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom OTLP File Exporter + opentelemetry updates #909

Merged
merged 12 commits into from
Feb 19, 2025

Conversation

joshwlewis
Copy link
Member

@joshwlewis joshwlewis commented Feb 6, 2025

As mentioned in #907, there was an upstream change to opentelemetry-stdout's exporter: open-telemetry/opentelemetry-rust#2040. With that change, the exporter lost the ability to send telemetry to a generic writer (like a buffer or file), and also lost jsonl serialization. This means we can't export in the correct format with newer versions of the opentelemetry libraries without a new exporter. This PR adds a custom OTLP File Exporter (spec is here) for traces and updates the opentelemetry libraries to the latest.

Based on this comment, and my upstream issue, there is interest in an OTLP File Exporter upstream, so this custom exporter shouldn't live here forever.

This exporter probably isn't quite good enough to upstream yet -- it supports traces, but not metrics or logs.

Bonus changes:

  • Now uses batch export functionality, which is generally preferred according to the spec. In opentelemetry-sdk 0.28, the batch export functionality no longer requires an async runtime (like tokio) and instead runs in a separate background thread (some details here), which lowers the dependency overhead for us.
  • Slightly better feature usage -- we're no longer compiling the metrics and logs features from opentelemetry, which we don't use anyway
  • This uses a LineWriter rather than a BufWriter now. The former is better for our use case -- we want to write complete lines to the file to prevent writing partial json to the file and corrupting it.
  • There were a truckload of random API changes in opentelemetry. Some things were moved, others renamed. Almost everything uses the builder pattern now.

@joshwlewis joshwlewis marked this pull request as ready for review February 13, 2025 00:41
@joshwlewis joshwlewis requested a review from a team as a code owner February 13, 2025 00:41
Copy link
Contributor

@colincasey colincasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshwlewis This looks good but, as we discussed, it will need to also mark the span passed into BuildpackTrace as the "active span" for later use in our individual buildpacks:
https://docs.rs/opentelemetry/latest/opentelemetry/trace/fn.mark_span_as_active.html

@joshwlewis
Copy link
Member Author

@colincasey I'll add the global span in another PR since it's sort of a new feature for libcnb.rs, where this is more of a dependency upgrade.

@joshwlewis joshwlewis merged commit 012e6b6 into main Feb 19, 2025
4 checks passed
@joshwlewis joshwlewis deleted the jwl/otlp-file-exporter branch February 19, 2025 21:43
"opentelemetry_sdk/trace",
"dep:opentelemetry-proto",
"opentelemetry-proto/trace",
"opentelemetry-proto/gen-tonic-messages",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's probably not much we can/should do about it, but it seems the gen-tonic-messages feature pulls in a whole load of additional deps (tonic, tower, prost etc), eg:
https://github.com/heroku/buildpacks-procfile/pull/262/files#diff-13ee4b2252c9e516a0547f2891aa2105c3ca71c6d7a1e682c69be97998dfc87eR1078

(I was hoping after open-telemetry/opentelemetry-rust#1569 updating to newer OTel might see a saving in buildpack crates, but the dependency count has increased).

Anyway, we need this functionality (and writing the transformation implementation ourselves would be worse), so there's not much we can do...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants