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

Add option to ignore invalid parent spans #1178

Merged
merged 4 commits into from
Aug 7, 2023

Conversation

cdavis-joy
Copy link
Contributor

What was changed

Adds an option to ignore errors when interpreting parent spans from headers.

Why?

When migrating from one tracing library to another, the new library will not find the parent span headers it expects, and will throw an errors, thus failing any workflows/activities that were running at the time of the worker deploy.

Checklist

  1. Closes Allow ignoring invalid SpanContext in OpenTelemetry TracingInterceptor #1174

  2. How was this tested:

Forced an error to return from t.readSpanFromHeader, and saw that error was not returned.

  1. Any docs updates needed?

Comment added to option.

@cdavis-joy cdavis-joy requested a review from a team as a code owner July 27, 2023 15:29
Copy link
Contributor

@Quinn-With-Two-Ns Quinn-With-Two-Ns left a comment

Choose a reason for hiding this comment

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

LGTM, will let @cretz take a look

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

@cdavis-joy - I think the better approach to supporting multiple tracer interceptors is to set the SpanContextKey and HeaderKey different for each. Then multiple tracing interceptors can be provided until you're ready to remove one.

But since this is opt-in, this addition is harmless. Will leave open to hear your thoughts on just setting different keys, then will merge.

@cdavis-joy
Copy link
Contributor Author

@cretz that's a good call. I would agree with you, but I'm failing to understand why were encountering an issue in the first place if the opentracing interceptor and the opentelemetry interceptor are using the same defaultHeaderKey of _tracer-data. After our worker using the opentelemtry tracer was deployed, we had a workflow fail with:

"failed extracting OpenTelemetry span from map",

That said, I appreciate the merge, and don't feel we need to dwell on this unless you know off the top of your head what we might be doing wrong to get that failure. 🙏

@cretz
Copy link
Member

cretz commented Jul 31, 2023

I'm failing to understand why were encountering an issue in the first place if the opentracing interceptor and the opentelemetry interceptor are using the same defaultHeaderKey of _tracer-data.

Because _tracer-data has OpenTracing-format serialized span, so it can't deserialize into an OpenTelemetry span.

@Quinn-With-Two-Ns Quinn-With-Two-Ns force-pushed the non-fatal-tracing-errors branch 2 times, most recently from d8d4778 to 957d834 Compare August 1, 2023 13:16
@cdavis-joy cdavis-joy force-pushed the non-fatal-tracing-errors branch from 957d834 to 9d3a161 Compare August 2, 2023 18:19
@cdavis-joy
Copy link
Contributor Author

Missed adding this to the otel interceptors Options func. Updated.

@cdavis-joy
Copy link
Contributor Author

@cretz I misunderstood what you initially said. By changing the HeaderKey and SpanContextKey, the newer interceptor would not even attempt to interpret spans from the old interceptor, thus avoiding these errors. Thanks for that, I think we're gonna give that a shot.

@cdavis-joy
Copy link
Contributor Author

This is something I'd still like to see as an option. It looks like these integration test failures are unrelated though?

@cretz
Copy link
Member

cretz commented Aug 7, 2023

Let me do a re-run and see. If/when it passes, we'll merge.

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.

Allow ignoring invalid SpanContext in OpenTelemetry TracingInterceptor
3 participants