-
-
Notifications
You must be signed in to change notification settings - Fork 164
feat(otel): add OpenTelemetry SpanProcessor, Propagator, Extractor #779
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
base: master
Are you sure you want to change the base?
Conversation
Seems like Windows CI is getting stuck on |
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.
Just gave this a quick glance and left some comments on what I noticed so far.
Thank you! |
Apparently we need to put each test in a different file or they will get stuck on Windows. |
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.
this looks reasonable to me.
Its a bit unfortunate how the lack of "onlyspans" means that you have to still deal with needing access to a TransactionOrSpan
object instead of just being able to push finished spans with opaque parent ids into a global span sink.
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.
lgtm
[dependencies] | ||
sentry-core = { version = "0.37.0", path = "../sentry-core", features = [ | ||
"client", | ||
"release-health" |
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.
I still don’t understand why the otel sub-crate depends directly on release-health?
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.
You're right it doesn't make much sense.
I was thinking there might be a scenario where someone depends on just sentry-core
and sentry-opentelemetry
, so I've added it to make sure that release-health
is activated.
But actually users are always forced to depend on sentry
as well in order to call sentry::init
and effectively use the SDK, so they will choose whether to enable release-health
when adding the sentry
dependency itself.
I actually added this release-health
flag to many of the subcrates when introducing this feature flag based on the above though process, see e.g. https://github.com/getsentry/sentry-rust/blob/master/sentry-tracing/Cargo.toml
On the other hand, if we don't do this and people upgrade the dependency without checking the release notes, they might end up without release-health
and not know, which is not nice.
See for example https://github.com/rust-lang/crates.io/blob/main/Cargo.toml
if we remove release-health
from the subcrates then the feature will be deactivated with a configuration like this.
Could you please advise on what you think is best here? @Swatinem
Creates a new crate
sentry-opentelemetry
for integration with OTEL.The implementation is based on a SpanProcessor that creates and finishes Sentry spans on OTEL span start/end.
There's also a Propagator/Extractor that injects the metadata (e.g. HTTP headers) required for distributed tracing to work.
It's based on our implementation for JS and other SDKs (https://develop.sentry.dev/sdk/telemetry/traces/opentelemetry/).
The goal with this initial version is:
Feedback appreciated on:
SPAN_MAP
, general Rust stuff, additions to performance API (@Swatinem)There are some todo items which will be addressed in follow-up PRs (not necessarily in this release).