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

Main readme to have runnable example #1778

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

cijothomas
Copy link
Member

I am not sure if we should maintain a lengthy code in the main readme.. If we chose to, we need to make sure it is showing all signals, and is a runnable example.

(Personally, I am okay to remove this from main readme file, but open to suggestions)

@cijothomas cijothomas requested a review from a team as a code owner May 15, 2024 19:22
Copy link

codecov bot commented May 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.1%. Comparing base (595ad05) to head (3f97f94).

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1778   +/-   ##
=====================================
  Coverage   74.1%   74.1%           
=====================================
  Files        125     125           
  Lines      19481   19481           
=====================================
+ Hits       14452   14454    +2     
+ Misses      5029    5027    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +163 to +164
opentelemetry = { version="0.23", features = ["trace", "logs", "metrics"]}
opentelemetry_sdk = { version="0.23", features = ["rt-tokio", "metrics"]}
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
opentelemetry = { version="0.23", features = ["trace", "logs", "metrics"]}
opentelemetry_sdk = { version="0.23", features = ["rt-tokio", "metrics"]}
opentelemetry = { version = "0.23", features = ["trace", "logs", "metrics"]}
opentelemetry_sdk = { version = "0.23", features = ["rt-tokio", "metrics"]}

For consistency

@lalitb
Copy link
Member

lalitb commented May 20, 2024

The example could be bit overwhelming for newcomers due to its length. Will it make sense to split it in 3 relatively smaller examples for logs, metrics and traces ?

@TommyCpp
Copy link
Contributor

The example could be bit overwhelming for newcomers due to its length. Will it make sense to split it in 3 relatively smaller examples for logs, metrics and traces ?

Probably can link to smaller examples? I feel like if we split it up we will just repeat a lot of setups

@cijothomas
Copy link
Member Author

The example could be bit overwhelming for newcomers due to its length. Will it make sense to split it in 3 relatively smaller examples for logs, metrics and traces ?

I am not sure what would be a good way... Keeping 3 examples will make the main readme even longer..
The opentelemetry-stdout has single example, but split by signal feature...

Maybe we should steal something like https://github.com/tokio-rs/axum/tree/main/examples/readme ? We can keep an example in the examples directory, named readme, which can be referred from the readme.md file. It can ensure main readme.md file is still small, and example is always kept up-to-date...

@cijothomas cijothomas marked this pull request as draft May 20, 2024 16:51
@cijothomas
Copy link
Member Author

Marking to draft, as I need to decide if we can do any better than this.. Please keep sharing the feedback.

@TommyCpp
Copy link
Contributor

TommyCpp commented May 20, 2024

and example is always kept up-to-date

I don't think that's a small task given we are improving APIs on a daily basis. I'd rather we get a stable place first and then add more examples. We cut down lots of examples earlier to keep the maintaince burdan low

@lalitb
Copy link
Member

lalitb commented May 20, 2024

I feel like if we split it up we will just repeat a lot of setups

We can a common toml for all 3 examples. Apart from that setup would be different in all three.

@cijothomas
Copy link
Member Author

and example is always kept up-to-date

I don't think that's a small task given we are improving APIs on a daily basis. I'd rather we get a stable place first and then add more examples. We cut down lots of examples earlier to keep the maintaince burdan low

We are already keeping example in stdout/otlp up-to-date! (it is also a good way for PR reviewers to see the impact of breaking changes, as we ourselves show the changes required in the example. Given most changelogs link to PR#, it also helps end users to know what changes they must do in their own project).

Not very sustainable path, but once we announce stable release, this should be resolved. Until then a lot of users will be 😞.

@TommyCpp
Copy link
Contributor

and example is always kept up-to-date

I don't think that's a small task given we are improving APIs on a daily basis. I'd rather we get a stable place first and then add more examples. We cut down lots of examples earlier to keep the maintaince burdan low

We are already keeping example in stdout/otlp up-to-date! (it is also a good way for PR reviewers to see the impact of breaking changes, as we ourselves show the changes required in the example. Given most changelogs link to PR#, it also helps end users to know what changes they must do in their own project).

Not very sustainable path, but once we announce stable release, this should be resolved. Until then a lot of users will be 😞.

Just to be clear I am not against the idea of breaking changes on APIs now or adding more examples. My worries are given we haven't stable yet, adding too many examples will add unnecessary burdan on us as maintainers, as well as contributors.

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.

None yet

3 participants