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

Env over compling time config #1323

Merged

Conversation

TommyCpp
Copy link
Contributor

@TommyCpp TommyCpp commented Oct 29, 2023

Fixes #1225

Changes

  • add document to show we favor env vars over compiling time values
  • correct Jaeger exporter builders to use the correct configuration methods

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • [] Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Files Coverage Δ
opentelemetry-jaeger/src/exporter/mod.rs 57.3% <100.0%> (-0.8%) ⬇️
opentelemetry-jaeger/src/exporter/agent.rs 14.8% <57.1%> (-0.7%) ⬇️
opentelemetry-jaeger/src/exporter/config/agent.rs 35.5% <87.0%> (+1.5%) ⬆️
opentelemetry-jaeger/src/exporter/runtime.rs 0.0% <0.0%> (ø)
...emetry-jaeger/src/exporter/config/collector/mod.rs 65.5% <94.9%> (+13.4%) ⬆️

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@TommyCpp TommyCpp marked this pull request as ready for review October 30, 2023 02:05
@TommyCpp TommyCpp requested a review from a team October 30, 2023 02:05
Copy link
Contributor

@shaun-cox shaun-cox left a comment

Choose a reason for hiding this comment

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

Thanks!

@TommyCpp
Copy link
Contributor Author

Will merge this after I added change logs

@cijothomas
Copy link
Member

While I understand that this is doing the agreed ordering of config priority, I don't think that we should keep doing anything for jaeger exporter. It should be marked deprecated, and removed just like https://github.com/open-telemetry/opentelemetry-rust/tree/main/opentelemetry-dynatrace was cleaned up!

@TommyCpp
Copy link
Contributor Author

TommyCpp commented Nov 1, 2023

While I understand that this is doing the agreed ordering of config priority, I don't think that we should keep doing anything for jaeger exporter. It should be marked deprecated, and removed just like https://github.com/open-telemetry/opentelemetry-rust/tree/main/opentelemetry-dynatrace was cleaned up!

While I agree with this. I also want to make sure we are consistent for those basic principal within our creates(jaeger is still a popular choice)

@cijothomas
Copy link
Member

While I understand that this is doing the agreed ordering of config priority, I don't think that we should keep doing anything for jaeger exporter. It should be marked deprecated, and removed just like https://github.com/open-telemetry/opentelemetry-rust/tree/main/opentelemetry-dynatrace was cleaned up!

While I agree with this. I also want to make sure we are consistent for those basic principal within our creates(jaeger is still a popular choice)

Not sure how will this help anyone! Are you saying we are going to keep publish jaeger to crates.io? It is just opposite of what I was proposing!

Jaeger is popular does not mean we need this crate.

@TommyCpp
Copy link
Contributor Author

TommyCpp commented Nov 1, 2023

I think given the popularity, someone may have interests to keep maintaining it. I want to set the right direction for them

@TommyCpp
Copy link
Contributor Author

TommyCpp commented Nov 1, 2023

As for #995. We are still missing documentation and examples on how to switch right? I feel like that's something we need to figure out before deleting it

@cijothomas
Copy link
Member

As for #995. We are still missing documentation and examples on how to switch right? I feel like that's something we need to figure out before deleting it

As for #995. We are still missing documentation and examples on how to switch right? I feel like that's something we need to figure out before deleting it

If that is the primary concern, I think we should just address that problem, instead of keeping on fixing things in jaeger exporter!
(We have done something in that direction https://github.com/open-telemetry/opentelemetry-rust/pull/1022/files).

@cijothomas
Copy link
Member

I think given the popularity, someone may have interests to keep maintaining it. I want to set the right direction for them

I don't follow....? I consider these as things we should get rid asap, to keep razor focused on shipping stable components from the repo. Anything not in that direction is just distraction, taking away time from the core goal.

We should not be keeping this in the main repo. (I'll comment in the contrib repo issue, as discussed in the SIG call yesterday, to propose to make that happen sooner)

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Agree that this is doing the agreed change, but my vote is to avoid doing anything, even bug-fixes, to jaeger exporter other than deprecating it, hence blocking the PR (overridable by maintainers, if the decision is to keep jaeger)

@TommyCpp I have requested-changes, not because the change is bad 🤣 , but I prefer to not do anything to jaeger! Hope you understand.

@TommyCpp
Copy link
Contributor Author

TommyCpp commented Nov 1, 2023

Agree that this is doing the agreed change, but my vote is to avoid doing anything, even bug-fixes, to jaeger exporter other than deprecating it, hence blocking the PR (overridable by maintainers, if the decision is to keep jaeger)

@TommyCpp I have requested-changes, not because the change is bad 🤣 , but I prefer to not do anything to jaeger! Hope you understand.

No worries! I am glad you call it out. Thank you for keep us on the right direction of deprcating jaeger and make sure we prioritize the important stuff.

I think we aligened on get rid of Jaeger on the long term but may have some disagreeement on how to handle Jaeger before we delete it from our repo.

The way I see it we should provide life support for the jaeger exporter up to a point where we provided clear instruction on migration and delete the code from it.

IMHO it means we should not:

  • Support new use cases/add features
  • Improve on exsiting API
  • Fix bugs if it only affects small percentage of users

But we should still make fixes if something just not working for most of the people in jaeger and/or security issues.

However, if we feel that it's still too much effort that's not worth it. I'd be happy to revise the PR accordingly.

@cijothomas
Copy link
Member

However, if we feel that it's still too much effort that's not worth it. I'd be happy to revise the PR accordingly.

Since the code is already written, no need of reversing! You can go ahead and merge. (I hinted in my comment that maintainers can override my request-changes).

@TommyCpp
Copy link
Contributor Author

TommyCpp commented Nov 2, 2023

I hinted in my comment that maintainers can override my request-changes

Actually I don't think I can 🤣

But I also don't want to override anyone. I think it's a good opportunity to to gather more ideas on how do we treat jaeger exporter between now and when we delete it completely, and make sure we are consistent

Thoughts? @open-telemetry/rust-approvers

@lalitb
Copy link
Member

lalitb commented Nov 2, 2023

my two cents based on the experience in deprecating and removing Jaeger exporter in otel-cpp repo -

While the opentelemetry-jaeger package is still part of the main repository, we can commit to a basic minimal maintenance plan to prioritize fixing any substantial bugs that affect a significant number of users, but hold off on adding new features. It makes sense to mark the opentelemetry-jaeger package as deprecated (updating it's Readme.md, updating Cargo.toml or whatever else it takes). We can still do new releases for the deprecated packages, but less frequent, and only critical bug fixes.

@cijothomas
Copy link
Member

my two cents based on the experience in deprecating and removing Jaeger exporter in otel-cpp repo -

While the opentelemetry-jaeger package is still part of the main repository, we can commit to a basic minimal maintenance plan to prioritize fixing any substantial bugs that affect a significant number of users, but hold off on adding new features. It makes sense to mark the opentelemetry-jaeger package as deprecated (updating it's Readme.md, updating Cargo.toml or whatever else it takes). We can still do new releases for the deprecated packages, but less frequent, and only critical bug fixes.

https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-jaeger/src/lib.rs#L4-L6 We have added this warning in the last release. That is not sufficient, we need to do more to sway users away, but that requires some work. I think we should just prioritize that work, instead of actually fixing anything in jaeger exporter, even if it is a bug fix!

We have no reason to follow other languages model - because in most other languages Jaeger was shipped as stable and then eventually deprecated. They had to move slowly, aligned with spec itself, which initially marked feature-freeze, then deprecated and finally removing.

In our case, OTel Rust never shipped a stable Jaeger, and spec already removed Jaeger exporter. So the only thing we need is to do the actual removal itself, not any bug fix.

@lalitb
Copy link
Member

lalitb commented Nov 3, 2023

Yes agree, valid points. We should start working towards deprecation process as soon as possible, and should spend less time in bug fixes. I can work on it if not already taken, as a learning experience. My main concern is - there are more than 6 Million downloads of the package till now, which is next to OTLP exporter, and users may have taken dependency on it which could be hard to get rid of immediately. That's why even after deprecation release, we shouldn't remove it from repo immediately (probably keeping it for another 6 months), and allow for critical/security bug fixes if required. This is irrespective of the fact that we are not yet stable, and also shouldn't be a blocker to go stable.

@cijothomas
Copy link
Member

Perhaps its best to go and merge this PR, as we are all aligned on the imminent Jaeger-Exporter deprecation. We can discuss how to do it smoothly, with minimal disruption to end users in #995

A good first would be to bring back the example that got lost during refactors.

@TommyCpp TommyCpp merged commit 4dd54a2 into open-telemetry:main Nov 6, 2023
15 checks passed
@hdost
Copy link
Contributor

hdost commented Nov 6, 2023

Started a discussion here #1341

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration tests Run integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion: priority on environment variable vs compiling time value
5 participants