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

Remove partial opentelemetry implementation #4086

Merged
merged 1 commit into from Jun 7, 2022

Conversation

ravangen
Copy link
Contributor

@ravangen ravangen commented Jun 2, 2022

Optional followup to #4077 and a comment impacting 2.0.9 and 1.13.13

This PR seeks to return this gem to a "stable state" and not have any misleading or inaccessible code. It is a revert of all the tracing changes with the exception of lib/graphql/tracing/platform_tracing.rb. I still believe there is some value in having a way to access the execution context when tracing, and this was the recommend way to achieve this per #4001.

First-party OpenTelemetry instrumentation would be wonderful, but it was not my intention to force this decision either way as part of the original suggested changes.

Apologies again for the confusion and happy to try and help sort this out based on your recommendations.

@rmosolgo
Copy link
Owner

rmosolgo commented Jun 3, 2022

Likewise, sorry I misunderstood the nature of these files in the original PR 🤦 I have no preference whether they're here or in opentelemetry-ruby.

My only hangup, IIRC, the only test for passing trace_phase was the opentelemetry tracer itself. If we remove it here, then an over-enthusiastic refactor might remove that option, since it's not used anywhere in this repo.

Would you mind adding a note to the source (or a test, if you can think of a handy way to add one) to help me maintain compatibility in the future?

@rmosolgo rmosolgo merged commit 073163b into rmosolgo:master Jun 7, 2022
@rmosolgo rmosolgo added this to the 2.0.10 milestone Jun 7, 2022
@rmosolgo
Copy link
Owner

rmosolgo commented Jun 7, 2022

Added a comment in f4d99f6

@ravangen
Copy link
Contributor Author

ravangen commented Jun 7, 2022

I still will see about adding a test case soon, just to have that peace of mind. Been a bit distracted recently.

@toneymathews
Copy link
Contributor

Can we pls add these changes to 1.13.x as well? I see 1.13.14 still has the opentelemetry implementation.

@rmosolgo
Copy link
Owner

Oops -- just released this in 1.13.15, too!

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.

None yet

3 participants