Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

boto3sqs: Do not override propagator-determined key #1202

Closed
Oberon00 opened this issue Jul 25, 2022 · 4 comments · Fixed by #1234
Closed

boto3sqs: Do not override propagator-determined key #1202

Oberon00 opened this issue Jul 25, 2022 · 4 comments · Fixed by #1234

Comments

@Oberon00
Copy link
Member

Oberon00 commented Jul 25, 2022

The boto3sqs instrumentation prepends "otel." to all attributes it adds to SQS messages (relevant code:

# We use this prefix so we can request all instrumentation MessageAttributeNames with a wildcard, without harming
# existing filters
_OPENTELEMETRY_ATTRIBUTE_IDENTIFIER: str = "otel."
_OTEL_IDENTIFIER_LENGTH = len(_OPENTELEMETRY_ATTRIBUTE_IDENTIFIER)
class Boto3SQSGetter(Getter[CarrierT]):
def get(self, carrier: CarrierT, key: str) -> Optional[List[str]]:
value = carrier.get(f"{_OPENTELEMETRY_ATTRIBUTE_IDENTIFIER}{key}", {})
if not value:
return None
return [value.get("StringValue")]
def keys(self, carrier: CarrierT) -> List[str]:
return [
key[_OTEL_IDENTIFIER_LENGTH:]
if key.startswith(_OPENTELEMETRY_ATTRIBUTE_IDENTIFIER)
else key
for key in carrier.keys()
]
class Boto3SQSSetter(Setter[CarrierT]):
def set(self, carrier: CarrierT, key: str, value: str) -> None:
# This is a limitation defined by AWS for SQS MessageAttributes size
if len(carrier.items()) < 10:
carrier[f"{_OPENTELEMETRY_ATTRIBUTE_IDENTIFIER}{key}"] = {
). This will make it impossible to write a propagator that works with instrumentations (of OTel or other vendors) that don't use this prefix.

The reasons cited at https://www.oxeye.io/blog/diving-into-opentelemetrys-specs:

So I suggest to remove the otel. prefix and instead use TextMapPropagator.fields to ensure the required attributes are fetched.

CC @oxeye-nikolay

@Oberon00

This comment was marked as resolved.

@melvinkcx
Copy link

melvinkcx commented Aug 22, 2022

Hi, I stumbled upon this while looking for the right way of "propagating" context across queues and other asynchronous means. It doesn't seem like the spec here covers such use cases. It is apparent that we already have a divergence in handling such propagation.

Just a suggestion, wouldn't it makes more sense if we could somehow agree upon the details and make it part of the spec (or recommendations)? I'm not familiar with how things work when it comes to OTEL spec, any insights will be much appreciated =)

@Oberon00
Copy link
Member Author

It is apparent that we already have a divergence in handling such propagation.

This instrumentation is the first I have seen that came up with modifying the key names determined by the propagator. There is discussion on context propagation for messaging in general at open-telemetry/oteps#192, but it doesn't cover the particular details about how to transport the context either. You could add a comment there.

@oxeye-nikolay
Copy link
Member

Hey, I apologize it took me a while to get to it.
I am very happy this is being brought up. It is an issue I was discussing a lot and honestly, no one has brought up the TextMapPropagator.fields solution. It still doesn't handle the AWS/Amazon prefix issue, but as you said it is less likely.
Let's get this PR merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants