Add Dataloader instrumentation library #137
Conversation
| end | ||
|
|
||
| def handle_event(@run_stop, _measurements, metadata, config) do | ||
| parent_ctx = OpentelemetryProcessPropagator.fetch_parent_ctx(4, :"$callers") |
There was a problem hiding this comment.
I found this unexpected. Why is this and the attach needed when immediately following the current span is set from metadata and ended?
There was a problem hiding this comment.
Good call. It was my first time working with OpentelemetryTelemetry and I was fiddling with it to get the right results. The current way showed me the correct spans in honeycomb but these should me moved to the @run_start event right and yield the same results right?
There was a problem hiding this comment.
Right. In your fiddling were you starting a span that is active when Dataloader is called, as in, does the first dataloader span have a parent? I'd expect it not to in the current setup.
|
Whats the status on this PR? Anything someone can assist with? |
| end | ||
|
|
||
| def handle_event(@run_stop, _measurements, metadata, config) do | ||
| OpentelemetryTelemetry.set_current_telemetry_span(config.tracer_id, metadata) |
There was a problem hiding this comment.
I don't think you need to do this.
There was a problem hiding this comment.
I got that from here:
Should it also be removed in the @batch_stop handle_event?
There was a problem hiding this comment.
Oh, and now I see its also in Phoenix https://github.com/open-telemetry/opentelemetry-erlang-contrib/blob/main/instrumentation/opentelemetry_phoenix/lib/opentelemetry_phoenix.ex#L136
@bryannaegele you'll have to explain this one to me :)
There was a problem hiding this comment.
@tsloughter it's possible another span is open, especially with Phoenix due to when controller functions return, so it was a safeguard to ensure the Phoenix span was closed properly to not lose the trace. I don't know that this is as much of a concern in other libraries, so probably not needed, but also isn't harmful if someone used it.
The other use case would be if some process was creating many spans that are getting closed asynchronously and you therefore have more than one in progress. An example would be a genserver.
I can't speak to the dataloader library, but those are the cases when I'd employ that function.
There was a problem hiding this comment.
Ok, yea, I'm guessing I thought OpentelemetryTelemetry worked differently when I originally commented.
|
@olivermt it is moving along. I just had another comment, it may be mergable soon. |
What does this PR do?
The Dataloader package is a library often used in conjunction with the Absinthe GraphQL library to perform batch loading of data. This can be useful for efficiently querying databases using Ecto or other custom data sources. It is important to add instrumentation to Dataloader to be able to track and analyze the performance of data loads and identify potential issues such as slow data loads or N+1 queries.
I've looked at the conventions for naming the events but could not find any applicable but open to suggestions.