Add oban measurements to span #212
Conversation
|
I think this looks right. Curious why you chose microseconds? Is that based on other libraries or semantic conventions? |
@tsloughter yeah that is a good question, i do not have any super strong reason for that. i just noticed it's the default in some of the other libs and it's what is used by the defauly oban telemetry logger as well. i could also make it configurable if that is preferred |
Ecto is doing this but it's also user configurable. Are these measurements just passing the ecto times or calculating its own? |
these are not using ecto times but times calculated by oban. duration is the amount of time the oban job takes to execute and queue_time is the amount of time the job was waiting before being picked up. i modified the time units to be configurable to match how opentelemetry_ecto handles things |
|
@tsloughter @bryannaegele can you take another look at this when you have a chance? |
|
Hey, sorry, I should have replied earlier. I'm unsure about making time unit configurable (too bad Ecto did). It'd be better to just use a consistent time unit (not only across these libs but also whatever is used across otel languages) in my opinion. If there isn't a consistent unit already used for span attributes that becomes more difficult but if its configurable then we can't undo it later while we can always switch to configurable without it being a breaking change. Not blocking this on that opinion, but its my hesitation. |
This reverts commit ea864d1.
@tsloughter that's fair, i dont have any strong opinion on this so i reverted that change. microseconds seems to be used frequently, i dont think there is any specific pattern around that though |
|
|
||
| defp set_measurements_attributes(%{duration: duration, queue_time: queue_time}) do | ||
| OpenTelemetry.Tracer.set_attributes(%{ | ||
| :"messaging.oban.duration_us" => System.convert_time_unit(duration, :native, :microsecond), |
There was a problem hiding this comment.
@tsloughter based on you slack question about not extending semcon namespaces, do you think we should look at merging this and then do a single pass fixing that?
There was a problem hiding this comment.
Better if we don't add any that are incorrect.
So switching this to oban.messaging... is needed.
There really needs to be some semconv for jobs libraries instead of just using messaging. I've seen this same thing in other languages, like Ruby's que.
There was a problem hiding this comment.
Yeah. There's just other attributes with the issue so didn't want to end up with mixed naming
There was a problem hiding this comment.
I vote we keep it as is in this PR and normalize all of it in another one prior to cutting a release.
Adds measurements from oban as attributes on the span.