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

feat/proposal: Deprecate jaeger enabled #1316

Merged
merged 8 commits into from
Feb 6, 2023
Merged

Conversation

markphelps
Copy link
Collaborator

@markphelps markphelps commented Feb 3, 2023

Fixes: FLI-197
Fixes: #1310

Deprecates tracing.jaeger.enabled in favor of:

tracing:
  enabled: true
  backend: jaeger

@markphelps markphelps requested a review from a team as a code owner February 3, 2023 17:41
@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2023

Codecov Report

Merging #1316 (2d9b8ad) into main (0af41e9) will increase coverage by 0.41%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1316      +/-   ##
==========================================
+ Coverage   80.37%   80.78%   +0.41%     
==========================================
  Files          43       43              
  Lines        3307     3331      +24     
==========================================
+ Hits         2658     2691      +33     
+ Misses        518      512       -6     
+ Partials      131      128       -3     
Impacted Files Coverage Δ
internal/config/config.go 88.83% <ø> (ø)
internal/config/deprecations.go 100.00% <ø> (ø)
internal/config/cache.go 100.00% <100.00%> (ø)
internal/config/tracing.go 100.00% <100.00%> (ø)
internal/server/flag.go 81.92% <100.00%> (+3.61%) ⬆️
internal/server/rule.go 76.92% <100.00%> (+3.84%) ⬆️
internal/server/segment.go 79.16% <100.00%> (+4.16%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines 134 to 135
enabled?: bool | *false
exporter?: "jaeger" | *"jaeger"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
enabled?: bool | *false
exporter?: "jaeger" | *"jaeger"
enabled: bool | *false
exporter: "jaeger" | *"jaeger"

I think you can forgo making this optional if there is a default.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually with exporter you might be able to get away with just:

exporter?: jaeger

Which just says if you specify the key exporter it must be the value "jaeger".
And drop the default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i'll prob just leave this as is for now since we will very shortly allow others besides jaeger

Copy link
Contributor

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

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

Nice. One observation that I am not 💯 is necessary.

@markphelps
Copy link
Collaborator Author

@GeorgeMac I'm wondering if we should change exporter to be collector or maybe something more agnostic like backend or consumer ? backend might be nice as it mimics what we have in the cache config as well.

Looking at OTel's glossary, it seems to me that:

  • exporter is actually the library used to send the traces
  • collector seems specific to their OTLP implementation

WYT?

@GeorgeMac
Copy link
Contributor

@markphelps I like your reasoning there. backend sounds good. Other ideas could be destination or target.
But I like backend just as much.

Copy link
Contributor

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

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

Sweet.

@markphelps markphelps merged commit af7a0be into main Feb 6, 2023
@markphelps markphelps deleted the deprecate-jaeger-enabled branch February 6, 2023 17:50
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.

[FLI-197] OTel: Exporter config
3 participants