Update backend to use Traces 0.14.1 #2
Conversation
There was a problem hiding this comment.
@ioquatix Happy New Year!
I realize this PR is not ready for merge, but I think I'm a bit stuck on it. I am using the Datadog backend in production, but would like this backend to function in situations where Datadog is unavailable. There's probably a lot more that you'd want to do to bump this gem: I noticed in the datadog backend you're using sus and i believe rubocop for linting. I'm happy to help bring it up to speed with the other backends, just need some guidance, thanks!
| @@ -33,7 +33,7 @@ module OpenTelemetry | |||
| TRACER = ::OpenTelemetry.tracer_provider.tracer(Traces::Backend::OpenTelemetry.name, Traces::Backend::OpenTelemetry::VERSION) | |||
|
|
|||
| module Interface | |||
| def trace(name, attributes: nil, &block) | |||
| def trace(name, attributes: {}, &block) | |||
There was a problem hiding this comment.
attributes calls transform_keys below, which nil will not respond to.
There was a problem hiding this comment.
Yes, that makes sense, alternatively we could write attributes&.transform_keys. Not sure what makes more sense.
| @@ -44,18 +44,18 @@ def trace(name, attributes: nil, &block) | |||
| end | |||
| rescue Exception => error | |||
| span&.record_exception(error) | |||
| span&.status = ::OpenTelemetry::Traces::Status.error("Unhandled exception of type: #{error.class}") | |||
| span&.status = ::OpenTelemetry::Trace::Status.error("Unhandled exception of type: #{error.class}") | |||
There was a problem hiding this comment.
appears to be a namespace change within OpenTelemetry
| if span_context = span.context | ||
| state = baggage.values(context: span.context) |
There was a problem hiding this comment.
state was an unused variable and the baggage api has changed, if this is important I need to understand the otel baggage concept and implement.
There was a problem hiding this comment.
I don't think it's important. The goal is to create a context which can be sent over the wire, e.g. over an HTTP request or encoded into a database query comment etc.
| flags = 0 | ||
|
|
||
| if span_context.trace_flags.sampled? | ||
| flags |= Context::SAMPLED | ||
| end |
There was a problem hiding this comment.
I pulled this from the Datadog Backend. as it was implemented, the trace flags were expected to be provided as an integer, not an object.
| Traces.trace_context = new_context | ||
| expect(instance.my_span.context).to have_attributes( | ||
| trace_id: new_context.trace_id, | ||
| span_id: new_context.parent_id | ||
| ) |
There was a problem hiding this comment.
I tried to keep this spec file as similar as possible to the datadog backend spec given the differences between sus and rspec i might have missed some things but i did my best. I assume that would be preferred.
this specific spec is failing, because as far as i can tell, the trace context is not being updated with the fake context provided under test. However, I'm not exactly sure this is the intended behavior. I need some feedback here to understand what specific feature this is supporting.
| # It's updated on the next call to trace: | ||
| expect(span.context.trace_id).not_to eq(new_context.trace_id) |
There was a problem hiding this comment.
ditto on this spec, its failing, but I think my assertions are correct assuming i understand the feature supported.
|
Thanks for working on this. Maybe we can do a 2-phase update. Firstly, let me rework the testing and update the gem code, then we can fix the open telemetry integration issues. I don't personally use Otel, so it would be good to have your guidance and feedback on those changes, along with any PRs you are willing to contribute (including this one, thanks!). |
|
Okay, I've modernised the main branch, do you mind rebasing on that? Sorry for then churn. |
jedschneider
force-pushed
the
update-0.14.1
branch
from
1066bab to
5f7a5bd
Compare
|
I've updated as much of the tests to incorporate your feedback, excluding the parts we don't know if they work or not. Please update the PR. I think that in order for the test assertions to work, OpenTelemetry might need to be put into a test mode, in order for us to track actual context assignment, and parent spans, etc. |
|
I've added support for |
PR to start to address #1 but I know this still needs a lot of work.
Types of Changes
Contribution