-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[processor/attributes] unable to add span custom attribute from custom header #5610
Comments
@jpkrohling sorry to bother you |
+1 Can't get any header from the request, but hardcoded key as in example above works
|
I can't understand why it's not working, it seemed to be ok just a bunch of monthes ago : |
@amoscatelli I saw you have two kinds of setting,
another is not with any prefix
Do these two settings both not work? |
@fatsheep9146 by sending traces to logging exporter I could verify that neither http.client_latitude or http.client_latitude_2 were added. Only 'test' key was present :
Which one is supposed to work ? With or without metadata prefix ? |
@amoscatelli |
@fatsheep9146 give me a minute, I'll give it another try |
Using this setting :
As you can see my OTEL receives requests with CloudFront-Viewer-Latitude header : Looking inside my logs :
This confirms that every from_context key attribute are not propagated/properly set, while the hardcoded/valued ones work properly. Please help ! |
If you have |
This is what I have :
I'll try to prepare a curl |
@jpkrohling here you are : I just added -H "CloudFront-Viewer-Latitude: 44.44444" and sending some spans generated by my application (copied directly from the payload of a traces request in browser DevTools). Was this what you needed ? |
@jpkrohling this is the output of tcpdump for a real request coming from my application :
As you can see CloudFront-Viewer-Latitude and CloudFront-Viewer-Longitude headers are present. Maybe User-Agent ? Is there any reason why metadata should not be filled with particular headers or in particular conditions ? |
New test :
Then this is logged :
So the problem is related ONLY to the specific CloudFront-Viewer-Latitude and CloudFront-Viewer-Longitude headers ... |
I guess maybe http header value should not be float ? Can you set CloudFront-Viewer-Latitude value to 41 to give it a try? |
Config :
Curl :
Log :
For god sake it seems a matter of NAME of the header ??? 😠 |
Yea, I confirm, the issue is related to the name of the header. 'Test' is ok, @jpkrohling |
I think I have enough information here to debug the actual code. I'll check and get back to you soon. |
Looks like I solved the mystery: the attribute is stored as receivers:
otlp:
protocols:
http:
include_metadata: true
exporters:
logging:
loglevel: debug
processors:
attributes:
actions:
- key: http.client_latitude2
from_context: metadata.X-Cloudfront-Viewer-Latitude
action: upsert
extensions:
service:
extensions:
pipelines:
traces:
receivers: [otlp]
processors: [attributes]
exporters: [logging] Resulting in:
|
THANK YOU very much @jpkrohling , going to test it right now ... EDIT : Yea, this is working properly ! Anyway this should be fixed, the match with the header should be ignore case !!! X-Cloudfront-Viewer-Latitude and x-cloudfront-viewer-latitude are the same http header, doing otherwise could lead to issues with libraries and sdk IMHO. |
I never worked with GO language, I don't know where to start, but if that's the case I can help of course |
We had a similar problem when it comes to the "authorization" header, and I thought about normalizing the keys either in lower or upper case: open-telemetry/opentelemetry-collector-contrib#8994 (comment) Having an accessor somewhere in the path would make it easy to access the normalized version from the context. |
Before this change, users would make references to headers such as "authorization" and "X-CloudFront-Viewer-Latitude", which would potentially fail, as they would be stored in the context as "X-Cloudfront-Viewer-Latitude" or "Authorization". This PR changes this behavior, to attempt a case insensitive lookup to the backing map in case the key wasn't found under the requested casing. Under the assumption that the lookup key specified by users won't change, on a subsequent lookup, we proactively copy the value to the looked up key, to make the next lookup faster. Fixes open-telemetry#5610 Fixes open-telemetry/opentelemetry-collector-contrib#8994 Signed-off-by: Juraci Paixão Kröhling <[email protected]>
I just submitted a new PR that would fix this: processors:
attributes:
actions:
- key: http.client_latitude
from_context: metadata.X-CloudFront-Viewer-Latitude
action: upsert
- key: http.client_latitude2
from_context: metadata.X-Cloudfront-Viewer-Latitude
action: upsert
|
Great ! |
Before this change, users would make references to headers such as "authorization" and "X-CloudFront-Viewer-Latitude", which would potentially fail, as they would be stored in the context as "X-Cloudfront-Viewer-Latitude" or "Authorization". This PR changes this behavior, to attempt a case insensitive lookup to the backing map in case the key wasn't found under the requested casing. Under the assumption that the lookup key specified by users won't change, on a subsequent lookup, we proactively copy the value to the looked up key, to make the next lookup faster. Fixes open-telemetry#5610 Fixes open-telemetry/opentelemetry-collector-contrib#8994 Signed-off-by: Juraci Paixão Kröhling <[email protected]>
* Change metadata lookup to be case insensitive Before this change, users would make references to headers such as "authorization" and "X-CloudFront-Viewer-Latitude", which would potentially fail, as they would be stored in the context as "X-Cloudfront-Viewer-Latitude" or "Authorization". This PR changes this behavior, to attempt a case insensitive lookup to the backing map in case the key wasn't found under the requested casing. Under the assumption that the lookup key specified by users won't change, on a subsequent lookup, we proactively copy the value to the looked up key, to make the next lookup faster. Fixes #5610 Fixes open-telemetry/opentelemetry-collector-contrib#8994 Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Add changelog entry Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Describe the bug
By enabling include_metadata and using attributes processor I don't see http headers from the POST request on the otlp http endpoint added as span attributes.
Steps to reproduce
I have set include_metadata to true in my http otlp receiver
I try to upsert a span custom attribute using attributes processor just like in this readme:
https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/confighttp/README.md
In my case the header is CloudFront-Viewer-Longitude but I don't think that's relevant :
Anyway below you'll find my full configuration.
What did you expect to see?
CloudFront-Viewer-Longitude added as http.client_longitude span attribute.
What did you see instead?
http.client_longitude is not added among span attributes.
What version did you use?
otel/opentelemetry-collector-contrib:0.54.0
What config did you use?
Environment
AWS Beanstalk with Docker Linux V2
Additional context
I need this to inject AWS Cloudfront geolocation headers into my spans :
https://aws.amazon.com/it/about-aws/whats-new/2020/07/cloudfront-geolocation-headers/
I have put my otel collector behind a non caching Cloudfront, and it is properly receiving POST http requests on v1/traces with those headers containing latitude and logitude.
I am sure everything is received properly because I am running tcpdump on 4318 port on otel collector and I DO see those headers.
Now I need to propagate those values from the http request, containing the spans in its body, to the span themself.
This is what include_metadata is supposed to enable, right ? Did I understand correctly ?
I also tried different combinations, trying to read from metadata context (inspired by https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/processor/attributesprocessor/testdata/config.yaml) but none worked.
The only attribute I see properly set is "test" to value 123, but that's the only key not using "from_context".
Please help.
The text was updated successfully, but these errors were encountered: