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

Populate client.Info from config helpers #4423

Merged
merged 7 commits into from
Dec 8, 2021

Conversation

jpkrohling
Copy link
Member

Fixes #4419

Signed-off-by: Juraci Paixão Kröhling [email protected]

@codecov
Copy link

codecov bot commented Nov 12, 2021

Codecov Report

Merging #4423 (b392242) into main (a06ca26) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4423      +/-   ##
==========================================
+ Coverage   90.67%   90.71%   +0.03%     
==========================================
  Files         179      181       +2     
  Lines       10450    10460      +10     
==========================================
+ Hits         9476     9489      +13     
+ Misses        757      755       -2     
+ Partials      217      216       -1     
Impacted Files Coverage Δ
client/client.go 100.00% <ø> (+10.34%) ⬆️
receiver/otlpreceiver/internal/logs/otlp.go 100.00% <ø> (ø)
receiver/otlpreceiver/internal/metrics/otlp.go 100.00% <ø> (ø)
receiver/otlpreceiver/internal/trace/otlp.go 100.00% <ø> (ø)
config/configauth/serverauth.go 100.00% <100.00%> (ø)
config/configgrpc/configgrpc.go 94.47% <100.00%> (+0.55%) ⬆️
config/confighttp/clientinfohandler.go 100.00% <100.00%> (ø)
config/confighttp/confighttp.go 100.00% <100.00%> (ø)
internal/middleware/wrappedstream.go 100.00% <100.00%> (ø)
receiver/otlpreceiver/otlphttp.go 57.40% <100.00%> (-4.26%) ⬇️

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 a06ca26...b392242. Read the comment docs.

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 20, 2021
@jpkrohling jpkrohling removed the Stale label Nov 22, 2021
@jpkrohling jpkrohling marked this pull request as ready for review November 22, 2021 16:30
@jpkrohling jpkrohling requested a review from a team November 22, 2021 16:30
@jpkrohling jpkrohling force-pushed the jpkrohling/issue4419 branch 3 times, most recently from bada4a5 to 0f18ea4 Compare November 24, 2021 10:43
@jpkrohling
Copy link
Member Author

CI is currently failing due to #4470

@jpkrohling jpkrohling force-pushed the jpkrohling/issue4419 branch 2 times, most recently from fe705ae to 8813c6f Compare November 24, 2021 11:18
go.sum Outdated Show resolved Hide resolved
@jpkrohling
Copy link
Member Author

Rebased. @bogdandrutu, could you please review/merge?

@jpkrohling
Copy link
Member Author

This is ready for another review, @bogdandrutu.

@jpkrohling
Copy link
Member Author

@bogdandrutu, would you please review and merge this one?

@bogdandrutu
Copy link
Member

@jpkrohling will merge after contrib is updated with the latest breaking change to not have multiple breaking changes in the same time.

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Copy link
Member Author

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

After a conversation with the folks from the go-grpc-middleware project, the PingService isn't part of their public API and I shouldn't be using it. As such, there's no point in adding that dependency anymore.

From your last review, these are the main changes:

  • internal/middleware now has a WrappedStream type, based on your previous suggestion and on go-grpc-middleware implementation
  • the tests that were using the PingService are not using the mock trace server based on the OTLP Traces
  • a simple unit test was added to cover the streaming case, which was previously covered in a scenario closer to the real world with a real streaming service

internal/middleware/wrappedstream.go Show resolved Hide resolved
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
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.

Populate client.Info from config helpers
2 participants