feature: add open telemetry integration#3
Conversation
fcarreno-collab
force-pushed
the
feature/olt_middleware
branch
from
1a9f857 to
fff33eb
Compare
caseylocker
left a comment
caseylocker
left a comment
There was a problem hiding this comment.
You're defining randomString in api/tests/open_telemetry_instrumentation_tests.py but not using it:
class OpenTelemetryInstrumentationTest(APITestCase):
def randomString(self, str_len):
...
Wrapt was downgraded in the requirements.txt. Was that intentional?
wrapt==2.0.1 -> wrapt==1.17.3
@caseylocker I removed this code you are right, its not been used, this was to generate a random string for the access_token we use to authenticate the requests on the other endpoints but here that does not happen
@caseylocker it was on propuse because newer versions create dependency conflicts when we try to install the open telemetry instrumentation libraries |
caseylocker
left a comment
caseylocker
left a comment
There was a problem hiding this comment.
LGTM. I've confirmed that the wrapt downgrade was a requirement by the Opentelementry instrumentation packages. They explicitly require wrapt<2.0.0. No other packages require wrapt 2.x
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@fcarreno-collab please review comments
|
@smarcet please can you review this. |
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@fcarreno-collab please review
fcarreno-collab
force-pushed
the
feature/olt_middleware
branch
from
36aa1c2 to
5ed2630
Compare
|
@smarcet changes related your comments already on PR |
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@fcarreno-collab please review comments
|
@smarcet branch updated with requested changes |
feature: move settings import and update tests
matiasperrone-exo
force-pushed
the
feature/olt_middleware
branch
from
36e886e to
221f64b
Compare
|
@smarcet please review again as all the requested/agreed changes were incorporated. |
| return | ||
|
|
||
| # Attach CF-Ray header | ||
| cf_ray = span.set_attribute("cf.ray_id", request.headers.get("Cf-Ray")) |
There was a problem hiding this comment.
span.set_attribute() returns None, so cf_ray is always falsy and the second attribute is never set. The cf.ray_id attribute is written correctly (as a side-effect of the first call), so
the test test_cf_ray_header passes despite this bug, masking it.
|
|
||
| DEV_EMAIL = os.getenv('DEV_EMAIL') | ||
|
|
||
| OTEL_INSTRUMENTATION_ENABLED = env_bool('OTEL_INSTRUMENTATION_ENABLED', True) |
There was a problem hiding this comment.
Any environment without this variable explicitly set to false will try to instrument. If OTEL_EXPORTER_OTLP_ENDPOINT is not configured, otel_endpoint mode will fail silently or log errors
on every request. Defaulting to False is safer.
|
It's ready to review @romanetar |
9 participants
ref: https://app.clickup.com/t/86b7bdn9g