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

Upgrade to use released version of Opentelemetry and also pushes the span context to gracefully handle multiple routers #4

Merged
merged 8 commits into from
Jul 13, 2022

Conversation

paulanthonywilson
Copy link

@paulanthonywilson paulanthonywilson commented Jul 13, 2022

Based on this unmerged PR to the base, but seemingly abandoned, original repo opentelemetry-beam#15 . In addition to what is advertised in the PR it updates to depend on the actually released version of Opentelemetry which reduces the dependency hell when trying to upgrade a services dependencies.

Small changes to the PR are:

  • A rebase to the current master
  • Loosen dependency requirements to make it easier to include with other deps.

From the original PR:

In the previous version, I did not anticipate the possibility that one router
might delegate to another for code-organization reasons, and so we would enter
the telemetry handler code twice while trying to service a request and lose the
parent context we attempted to save on the first time around. We witnessed this
as traces in our aggregator/visualizer that were missing root spans.

This change brings the code closer to what opentelemetry_telemetry does without
adding that dependency; namely, it maintains the parent context as a list/stack
and pushes onto it when a new span is created, popping it when a span completes.
Also, if a context already exists, we don't attempt to extract propagation from
the request headers. This change will also make the library more compatible with
webserver instrumentation like opentelemetry_cowboy, which does the header
extraction and context creation.

seancribbs and others added 8 commits July 13, 2022 10:29
In the previous version, I did not anticipate the possibility that one router
might delegate to another for code-organization reasons, and so we would enter
the telemetry handler code twice while trying to service a request and lose the
parent context we attempted to save on the first time around. We witnessed this
as traces in our aggregator/visualizer that were missing root spans.

This change brings the code closer to what opentelemetry_telemetry does without
adding that dependency; namely, it maintains the parent context as a list/stack
and pushes onto it when a new span is created, popping it when a span completes.
Also, if a context already exists, we don't attempt to extract propagation from
the request headers. This change will also make the library more compatible with
webserver instrumentation like opentelemetry_cowboy, which does the header
extraction and context creation.
Copy link

@pedro-gutierrez pedro-gutierrez left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link

@jamcito jamcito left a comment

Choose a reason for hiding this comment

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

LGTM!

@paulanthonywilson paulanthonywilson merged commit 47b8f2c into master Jul 13, 2022
@paulanthonywilson paulanthonywilson deleted the push-span-ctx-forked-repo branch July 13, 2022 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants