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.

Sign up for GitHub

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

Jump to bottom

Document TraceConfig #1550

Merged
merged 5 commits into from
Aug 19, 2020
Merged

Document TraceConfig #1550

merged 5 commits into from
Aug 19, 2020

Conversation

malafeev
Copy link
Contributor

fix #1496

open-telemetry#1496 Document TraceConfig
c79d557 
Signed-off-by: Sergei Malafeev <sergei@malafeev.org>
fix missing character
e5adeb6 
Signed-off-by: Sergei Malafeev <sergei@malafeev.org>
@codecov
Copy link

codecov bot commented Aug 18, 2020 •
edited
Loading

Codecov Report

Merging #1550 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1550   +/-   ##
=========================================
  Coverage     91.45%   91.45%           
  Complexity      960      960           
=========================================
  Files           116      116           
  Lines          3440     3440           
  Branches        281      281           
=========================================
  Hits           3146     3146           
  Misses          205      205           
  Partials         89       89
Impacted Files Coverage Δ Complexity Δ
...telemetry/sdk/trace/export/BatchSpanProcessor.java 94.59% <0.00%> (-1.36%) 8.00% <0.00%> (ø%)
...y/sdk/metrics/aggregator/DoubleMinMaxSumCount.java 100.00% <0.00%> (+3.92%) 6.00% <0.00%> (ø%)

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 a08dd9f...a28297d. Read the comment docs.

iNikem
iNikem approved these changes Aug 18, 2020
View reviewed changes
QUICKSTART.md Outdated
| otel.config.sampler.probability | OTEL_CONFIG_SAMPLER_PROBABILITY | Sampler which is used when constructing a new span |
| otel.config.max.attrs | OTEL_CONFIG_MAX_ATTRS | Max number of attributes per span |
| otel.config.max.events | OTEL_CONFIG_MAX_EVENTS | Max number of Events per span |
| otel.config.max.links | OTEL_CONFIG_MAX_LINKS | Max number of Link} entries per span |
Copy link
Contributor

Choose a reason for hiding this comment

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

extra } after Link

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

QUICKSTART.md Outdated
| System property | Environment variable | Purpose |
|----------------------------------|----------------------------------|----------------------------------------------------|
| otel.config.sampler.probability | OTEL_CONFIG_SAMPLER_PROBABILITY | Sampler which is used when constructing a new span |
| otel.config.max.attrs | OTEL_CONFIG_MAX_ATTRS | Max number of attributes per span |
Copy link
Contributor

Choose a reason for hiding this comment

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

For all MAX attributes it would be useful to say what happens if that limit is exceeded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

javadoc doesn't say what happens, I suppose exceeded should be dropped.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, extra attributes/links/events will be dropped. Too long values will be truncated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@iNikem
Copy link
Contributor

iNikem commented Aug 18, 2020

Thanks for documentation PRs!

Sergei Malafeev added 2 commits August 18, 2020 23:02
get rid of }
782a61b 
Signed-off-by: Sergei Malafeev <sergei@malafeev.org>
for all MAX attributes added what happens if that limit is exceeded.
b20aa2e 
Signed-off-by: Sergei Malafeev <sergei@malafeev.org>
jkwatson
jkwatson reviewed Aug 18, 2020
View reviewed changes
QUICKSTART.md Outdated
@@ -402,6 +403,43 @@ tracerProvider.addSpanProcessor(BatchSpanProcessor.newBuilder(
).build());
```

### TraceConfig

`TraceConfig` associated with `TracerSdkProvider` can be updated via system properties,
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
`TraceConfig` associated with `TracerSdkProvider` can be updated via system properties,
The `TraceConfig` associated with a `TracerSdkProvider` can be updated via system properties,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@jkwatson
Copy link
Contributor

Looks good! Just one tiny update to the text desired. Thanks for the contribution!

@trask
Copy link
Member

trask commented Aug 18, 2020

Maybe document the default values? (and if you don't mind, a PR to sync https://github.com/open-telemetry/opentelemetry-java-instrumentation#trace-config)

add defaults and rewording
a28297d 
Signed-off-by: Sergei Malafeev <sergei@malafeev.org>
@malafeev
Copy link
Contributor Author

@trask I added default values

@carlosalberto carlosalberto merged commit 750707b into open-telemetry:master Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iNikem iNikem iNikem approved these changes

@trask trask trask approved these changes

@jkwatson jkwatson jkwatson approved these changes

@thisthat thisthat thisthat approved these changes

@anuraaga anuraaga Awaiting requested review from anuraaga

@arminru arminru Awaiting requested review from arminru

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu

@carlosalberto carlosalberto Awaiting requested review from carlosalberto

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

@tylerbenson tylerbenson Awaiting requested review from tylerbenson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Document TraceConfig
6 participants
@malafeev @iNikem @jkwatson @trask @thisthat @carlosalberto