-
Notifications
You must be signed in to change notification settings - Fork 8
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
Conversation
There was a problem hiding this 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
@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. |
"opentelemetry_sdk/trace", | ||
"dep:opentelemetry-proto", | ||
"opentelemetry-proto/trace", | ||
"opentelemetry-proto/gen-tonic-messages", |
There was a problem hiding this comment.
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...
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 lostjsonl
serialization. This means we can't export in the correct format with newer versions of theopentelemetry
libraries without a new exporter. This PR adds a custom OTLP File Exporter (spec is here) for traces and updates theopentelemetry
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:
opentelemetry-sdk
0.28
, the batch export functionality no longer requires an async runtime (liketokio
) and instead runs in a separate background thread (some details here), which lowers the dependency overhead for us.metrics
andlogs
features fromopentelemetry
, which we don't use anywayLineWriter
rather than aBufWriter
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.opentelemetry
. Some things were moved, others renamed. Almost everything uses the builder pattern now.