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

Extract client IP address and store it into context for OTLP/HTTP #4257

Merged
merged 1 commit into from
Oct 27, 2021

Conversation

pmm-sumo
Copy link
Contributor

Description:

Fixes otlphttp receiver so it adds information about the source IP into the context

Context: For k8sattributesprocessor the source IP is frequently required to associate the source with a given pod. This bug is emphasised now since SDK's are now encouraged to switch from gRPC to HTTP

Link to tracking Issue: #4256

Testing: Tested manually. I considered extending unit tests to cover this case, though I believe it would required including Context (as an array or the last one) in consumertest.TracesSink and I wasn't sure if it's a right thing to do.

Documentation: Not changed

@pmm-sumo pmm-sumo requested review from a team and bogdandrutu October 25, 2021 13:53
@pmm-sumo pmm-sumo changed the title Extract source IP address into context for OTLP/HTTP Extract client IP address and store it into context for OTLP/HTTP Oct 27, 2021
@codecov
Copy link

codecov bot commented Oct 27, 2021

Codecov Report

Merging #4257 (4503eb9) into main (46c8e22) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4257      +/-   ##
==========================================
+ Coverage   88.22%   88.24%   +0.01%     
==========================================
  Files         173      173              
  Lines       10320    10332      +12     
==========================================
+ Hits         9105     9117      +12     
  Misses        975      975              
  Partials      240      240              
Impacted Files Coverage Δ
receiver/otlpreceiver/otlphttp.go 61.66% <100.00%> (+4.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46c8e22...4503eb9. Read the comment docs.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Is there a test for this?

@pmm-sumo
Copy link
Contributor Author

Is there a test for this?

I think we would need to extend consumertest.TracesSink (and so on) with keeping the Context to be able to test it, which I wasn't sure if it's a right thing to do. We discussed during the SIG an idea of using additional fields introduced by auth-related code (I think this PR covers them: #3745)

If you believe we should add Context to test sinks, I will be happy to do that and add the tests

@bogdandrutu bogdandrutu merged commit 9488d4c into open-telemetry:main Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants