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

jaegerremote: Add WithSamplingStrategyFetcher option #4045

Merged
merged 16 commits into from
Sep 29, 2023
Merged

jaegerremote: Add WithSamplingStrategyFetcher option #4045

merged 16 commits into from
Sep 29, 2023

Conversation

morus12
Copy link
Contributor

@morus12 morus12 commented Jul 4, 2023

  • Fixed an issue where the fetcher would panic if the default transport is not of type *http.Transport due to the cast:

    customTransport := http.DefaultTransport.(*http.Transport).Clone()

    To address this, we've now set the http.Client.Timeout rather than customTransport.ResponseHeaderTimeout to prevent panic.

  • This PR exposes SamplingStrategyFetcher and WithSamplingStrategyFetcher, enabling the definition of a custom fetcher that utilizes a custom HTTP client with a transport other than *http.Transport. We have integrated httpmock, which clashes with the default http.Client. To address this, we employ custom HTTP clients in our fetcher implementation.

@morus12 morus12 requested a review from a team July 4, 2023 10:30
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 4, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@dmathieu
Copy link
Member

dmathieu commented Jul 4, 2023

Please add a unit test.

@MrAlias
Copy link
Contributor

MrAlias commented Jul 4, 2023

Please sign the CLA so these changes can be considered.

@MrAlias MrAlias added the blocked: CLA Waiting on CLA to be signed before progress can be made label Jul 4, 2023
@morus12 morus12 marked this pull request as draft July 5, 2023 10:37
@pellared pellared removed the blocked: CLA Waiting on CLA to be signed before progress can be made label Jul 13, 2023
@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #4045 (51c2c31) into main (9d4eb7e) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #4045   +/-   ##
=====================================
  Coverage   82.2%   82.2%           
=====================================
  Files        144     144           
  Lines      10005   10002    -3     
=====================================
  Hits        8226    8226           
+ Misses      1638    1636    -2     
+ Partials     141     140    -1     
Files Coverage Δ
samplers/jaegerremote/sampler_remote.go 89.4% <100.0%> (-0.3%) ⬇️
samplers/jaegerremote/sampler_remote_options.go 100.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

@morus12 morus12 marked this pull request as ready for review August 7, 2023 21:25
CHANGELOG.md Outdated Show resolved Hide resolved
pellared
pellared previously approved these changes Aug 8, 2023
@pellared pellared dismissed their stale review August 8, 2023 13:31

It looks like Jaeger sampling is deprecated

pellared
pellared previously approved these changes Aug 8, 2023
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

PS. Only "Client Sampling" is deprecated https://www.jaegertracing.io/docs/1.47/sampling/

hanyuancheung
hanyuancheung previously approved these changes Aug 24, 2023
@morus12 morus12 requested a review from yurishkuro as a code owner September 1, 2023 11:28
yurishkuro
yurishkuro previously approved these changes Sep 1, 2023
@pellared
Copy link
Member

pellared commented Sep 5, 2023

This branch is out-of-date with the base branch

@morus12 Can you please update the branch?

@pellared pellared dismissed their stale review September 6, 2023 08:00

comments need adressing

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Few things needs to be addressed.

@pellared
Copy link
Member

@morus12 Do you plan to update the PR and address the comments?

@morus12
Copy link
Contributor Author

morus12 commented Sep 26, 2023

  1. I added comments.
  2. Set the defaultRemoteSamplingTimeout only when http.DefaultClient.Transport is *http.Transport.
  3. Test if defaultRemoteSamplingTimeout is set when conditions are met.
  4. Test if there is no panic with custom http.DefaultClient.Transport.

Let me know if you need any other changes.

CHANGELOG.md Outdated Show resolved Hide resolved
samplers/jaegerremote/sampler_remote.go Outdated Show resolved Hide resolved
@pellared pellared dismissed stale reviews from yurishkuro and hanyuancheung September 26, 2023 08:27

PR has significant changes.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
morus12 and others added 2 commits September 26, 2023 14:35
@morus12 morus12 requested a review from pellared September 26, 2023 13:10
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Waiting for codeowner (@yurishkuro) review.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Lgtm, although I don't follow the need to clone the transport.

samplers/jaegerremote/sampler_remote.go Outdated Show resolved Hide resolved
@pellared
Copy link
Member

@morus12 Please address #4045 (comment) and update the branch and I will merge the PR 🎉

@pellared pellared changed the title expose SamplingStrategyFetcher interface jaegerremote: Add WithSamplingStrategyFetcher option Sep 29, 2023
@pellared pellared merged commit b3569d7 into open-telemetry:main Sep 29, 2023
26 checks passed
@pellared
Copy link
Member

@morus12 Thank you for your contribution and have a nice weekend 😉

pellared added a commit to pellared/opentelemetry-go-contrib that referenced this pull request Sep 29, 2023
pellared added a commit that referenced this pull request Sep 29, 2023
@morus12
Copy link
Contributor Author

morus12 commented Oct 2, 2023

I appreciate your help!
It was an exciting challenge, maybe I'll pick up another issue ;)

@morus12 morus12 deleted the public-fetcher-interface branch October 2, 2023 07:37
@pellared pellared added this to the untracked milestone Nov 8, 2024
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.

6 participants