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

[webhookeventreceiver] add option to include headers as attributes #37815

Merged
merged 6 commits into from
Mar 10, 2025

Conversation

tredman
Copy link
Contributor

@tredman tredman commented Feb 10, 2025

Description

Hi! I'm looking to add a feature to the webhookeventreceiver to allow HTTP request headers to be passed through as log attributes. Our initial use case for this is for receiving webhook events from Github/Github Enterprise - which stores important metadata in the webhook request headers that is absent from the payload.

This PR adds a new option, convert_headers_to_attributes, which will include any headers except the one specified in required_header in the payload as an attribute.

Link to tracking issue

Fixes

N/A - started with this PR

Testing

Added unit tests and I'm running this successfully in our environment using a local build.

Documentation

README

@tredman tredman requested review from atoulme and a team as code owners February 10, 2025 22:17
Copy link

linux-foundation-easycla bot commented Feb 10, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@shalper2
Copy link
Contributor

shalper2 commented Feb 11, 2025

Hey this is really great! A couple of small things: First in factory.go the default config should be updated to include false for the new setting (since it is optional). Second, could you add two additional test cases for me, one which covers the normalizeHeader function and another where convert_headers_to_attributes is set but no non-required headers are passed in? other than that I think this looks good!

@tredman
Copy link
Contributor Author

tredman commented Feb 11, 2025

Thanks for the quick review and feedback! I think I've addressed it in my latest commit but let me know.

re: normalizeHeader - i renamed this to be a little more descriptive of what it was doing: generating an OTEL attribute key from a header.

Copy link
Contributor

@shalper2 shalper2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! Thanks!

func headerAttributeKey(header string) string {
snakeCaseHeader := strings.ReplaceAll(header, "-", "_")
snakeCaseHeader = strings.ToLower(snakeCaseHeader)
return "header." + snakeCaseHeader
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to transform to snake case? That can be done later in a processor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was (maybe poorly) trying to conform to https://opentelemetry.io/docs/specs/semconv/general/naming/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that this is input data, not a deterministic piece of data. You can have collision right now such as two headers with the names “content-length” and “Content Length”. It’s better to let processors perform data sanitization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. For our use case it would indeed be easy enough to do this in a processor as you say, without baking it in here. What do you think about maintaining the header. namespace? I think it would still be useful to differentiate the attributes this way.

@tredman
Copy link
Contributor Author

tredman commented Feb 15, 2025

Updated PR to address feedback on multi-values and switch to using regex instead. I also added a comment to headerAttributeKey to hopefully clarify the attribute key naming

Switching to regex was a little more complex, since I wanted to ensure the regex was validated on startup and also pass the compiled regex down to reqToLog. I made a couple of changes:

  • reqToLog is now a receiver function, receiving an eventReciever - this simplifies the function header but does mean it's a little more complex to test. I think this makes sense because reqToLog requires several values from the calling eventReceiver anyway
  • newLogsReceiver now calls Config.Validate() - this was previously only referenced in tests, and there was some duplicated validation code. I added a couple of test cases for some previously untested max values. Validate() now also checks the regex.

@tredman tredman force-pushed the webhook-headers branch 2 times, most recently from 5e98f29 to ee0e3e1 Compare February 15, 2025 15:38
@mterhar
Copy link
Member

mterhar commented Feb 18, 2025

I opened another PR against the same receiver. When this merges, I'll rebase. #38042

@tredman
Copy link
Contributor Author

tredman commented Mar 4, 2025

👋 Pushed a rebase after the last PR was merged. Is there any feedback here that still needs to be addressed?

@atoulme atoulme added the ready to merge Code review completed; ready to merge by maintainers label Mar 5, 2025
Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated code is out of date, please run "make generate" and commit the changes in this PR.

also please fix lint http://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/13655296572/job/38267389141?pr=37815

@songy23 songy23 removed the ready to merge Code review completed; ready to merge by maintainers label Mar 7, 2025
tredman added 6 commits March 7, 2025 13:11

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
- set ConvertHeadersToAttributes to false in `createDefaultConfig`
- Adds additional test cases for ReqToLog
- Adds tests for normalized header attribute key
@tredman
Copy link
Contributor Author

tredman commented Mar 7, 2025

ran make generate and fixed lint errors

@tredman tredman requested a review from songy23 March 10, 2025 20:56
@songy23 songy23 merged commit 9d6b1cb into open-telemetry:main Mar 10, 2025
170 of 171 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants