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

OpenTelemetryTracerBridge.Start: Make returned context contain span #6614

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

aknuds1
Copy link
Contributor

@aknuds1 aknuds1 commented Nov 10, 2023

What this PR does

I found that there's a bug in OpenTelemetryTracerBridge.Start, i.e. that the returned context doesn't contain a go.opentelemetry.io/otel/trace.Span although it should. This PR fixes it, and adds a test for it.

The practical effect of the bug is losing OpenTelemetry tracing events, e.g. in Prometheus, where code depends on fetching a span from the context (if no span is in the context, these events are discarded).

Which issue(s) this PR fixes or relates to

Checklist

  • Tests updated
  • [na] Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@aknuds1 aknuds1 added bug Something isn't working component/tracing labels Nov 10, 2023
@aknuds1 aknuds1 marked this pull request as ready for review November 10, 2023 16:00
@aknuds1 aknuds1 requested a review from a team as a code owner November 10, 2023 16:00
@aknuds1 aknuds1 requested review from ying-jeanne and a team November 10, 2023 18:29
Copy link
Contributor

@ying-jeanne ying-jeanne left a comment

Choose a reason for hiding this comment

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

LGTM

@aknuds1 aknuds1 merged commit 1408bdc into main Nov 13, 2023
27 checks passed
@aknuds1 aknuds1 deleted the arve/tracing branch November 13, 2023 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component/tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants