Otel Common Code #1248
Otel Common Code #1248
Conversation
corlettb
force-pushed
the
BC-otel-mk5
branch
3 times, most recently
from
b1d0565 to
3d7d1a6
Compare
| set_tracer_provider(TracerProvider(resource=resource)) | ||
|
|
||
| def regex_predicate(baggage_key: str) -> bool: | ||
| return baggage_key.startswith("^public-.+") |
There was a problem hiding this comment.
This could do with some explanation. Why is it checking whether a string of some sort begins with a literal regex pattern? If it was cribbed from https://github.com/open-telemetry/opentelemetry-python-contrib/tree/main/processor/opentelemetry-processor-baggage#usage then I think that might be wrong - the code shows no special handling around regexes, it's just a pure string https://github.com/open-telemetry/opentelemetry-python-contrib/blob/80c357bb16b42c76763d85441e53dd4f6087d6c5/processor/opentelemetry-processor-baggage/src/opentelemetry/processor/baggage/processor.py#L61
There was a problem hiding this comment.
Previously I add all baggage getting added as an attribute. I thought that might be bad if we started passing PII in baggage so I added a regex. I didn't check to see if it works. I probably should have.
I could just remove the whole BaggageSpanProcessor if you want.
It's a case of me try to guess what might be useful to someone in the future rather than it being useful now.
There was a problem hiding this comment.
I've fixed the startswith. Let me know if you rather I removed entirely.
There was a problem hiding this comment.
I think I'd prefer ALLOW_ALL_BAGGAGE_KEYS - if we ever put PII in baggage then we've already screwed up and I would prefer to catch that during code review rather than relying on this regex to catch it
The different otel instrumentation are implemented as optional dependencies. Add a common config class that can be shared by applications to pick up common configuration.
- Add TODO comment about switching to dynamic service.name - Prefix variables with NOTIFY_ - Remove defaults from otel variables. Rely on the BaseConfig - Fix the baggage span processor regex - Remove the otel_span decorator - Rename otel_histogram to otel_histogram - Increase celery timeout to 45 seconds
| otel-requests = [ "opentelemetry-instrumentation-requests==0.54b1" ] | ||
| otel-redis = [ "opentelemetry-instrumentation-redis==0.54b1" ] | ||
| otel-sqlalchemy = [ "opentelemetry-instrumentation-sqlalchemy==0.54b1" ] | ||
| otel-botocore = [ "opentelemetry-instrumentation-botocore==0.54b1" ] |
There was a problem hiding this comment.
Pinned versions? What's the plan for keeping these updated?
There was a problem hiding this comment.
Manually updating them? I'm happy to go with what you want though.
https://trello.com/c/mmrH8QbC/1154-turn-second-otel-spike-into-a-pr-we-can-merge
What
Add otel common code.
The different otel instrumentation are implemented as optional dependencies.
Add a common config class that can be shared by applications to pick up common configuration.
We are not doing anything on the otel logging front yet.
Why
This is part of our push to get into more standard observability.