search

open-telemetry / opentelemetry-python-contrib

OpenTelemetry instrumentation for Python modules
https://opentelemetry.io
Apache License 2.0
647 stars 535 forks source link

Fix wrong unit in Flask new semconv test #2645

Open emdneto opened 3 days ago

emdneto commented 3 days ago

Description

Changes duration to be measured in seconds during the test since in stable semconv, the unit changed from ms to s and we are recording in seconds

Probably Fixes #2596

emdneto commented 3 days ago

I don't think this is the only test that has the same duration calculation

Do you mean in other instrumentations? If so, I think yes, that's right it might have other tests. For Flask, I believe this is the only one that uses new_semconv. I think the other tests in Flask are correct because they are using the default opt-in (old semconv), and the histogram unit is in ms

xrmx commented 3 days ago

I don't think this is the only test that has the same duration calculation

Do you mean in other instrumentations? If so, I think yes, that's right. For Flask, I think this is the only one that uses new_semconv. I think the other ones are correct because they are using the default opt-in (old semconv), and the histogram unit is in ms

Missed the setUp doing the patching based on test names :fearful: For the next conversion what about using a decorator so at least it's explicit that something is going on?

emdneto commented 3 days ago

Given this is now in seconds, isn't 10 seconds a bit too much for delta?

Makes sense