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

Updated Sampling Result names #1655

Closed
wants to merge 9 commits into from
Closed

Updated Sampling Result names #1655

wants to merge 9 commits into from

Conversation

euniceek
Copy link
Contributor

This PR updates the names of Sampling result for consistency, closing #1648

The changes have been made following these specifications:
open-telemetry/opentelemetry-specification#938
open-telemetry/opentelemetry-specification#956

Changed from:

  • NOT_RECORD

  • RECORD

  • RECORD_AND_SAMPLED

Changed to:

  • DROP

  • RECORD_ONLY

  • RECORD_AND_SAMPLE

@euniceek euniceek marked this pull request as ready for review September 16, 2020 17:51
jkwatson
jkwatson approved these changes Sep 16, 2020
View reviewed changes
Copy link
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

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

Looks great. Let's hold off on merging until the DROP spec PR is merged. Thanks!

anuraaga
anuraaga reviewed Sep 17, 2020
View reviewed changes
Copy link
Contributor

@anuraaga anuraaga left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @eunice98k - can you run ./gradlew googleJavaFormat to fix the formatting violations?

jkwatson and others added 2 commits September 17, 2020 13:07
@jkwatson
remote instructions on generating javadoc (open-telemetry#1654)
8470d6a 
add a link in the README to the examples
SDK: Remove Javadocs since tags (for pre 1.0 release) (open-telemetry…
51e93f7 
…#1650)

* Add since for SDK (corresponding to the version in API)

* Add since based on release branches

* Remove all since tags in SDK
@thisthat thisthat added the blocked:spec blocked on open or unresolved spec label Sep 17, 2020
thisthat
thisthat approved these changes Sep 17, 2020
View reviewed changes
Copy link
Member

@thisthat thisthat left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution :)

@Oberon00
Copy link
Member

@thisthat No need to add blocked:spec, open-telemetry/opentelemetry-specification#956 is merged already.

@arminru arminru removed the blocked:spec blocked on open or unresolved spec label Sep 17, 2020
@thisthat
Copy link
Member

@thisthat No need to add blocked:spec, open-telemetry/opentelemetry-specification#956 is merged already.

oh yeah! My bad! I blindly read the comment of @jkwatson and add the label without a double check!

@jkwatson
Copy link
Contributor

@eunice98k looks like this needs a rebase, and a run of the formatter, then should be good to go. Thanks!

@codecov
Copy link

codecov bot commented Sep 17, 2020

Codecov Report

Merging #1655 into master will increase coverage by 0.08%.
The diff coverage is 91.89%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1655      +/-   ##
============================================
+ Coverage     86.62%   86.70%   +0.08%     
- Complexity     1398     1402       +4     
============================================
  Files           163      164       +1     
  Lines          5524     5545      +21     
  Branches        552      554       +2     
============================================
+ Hits           4785     4808      +23     
+ Misses          542      541       -1     
+ Partials        197      196       -1
Impacted Files Coverage Δ Complexity Δ
...in/java/io/opentelemetry/metrics/DefaultMeter.java 100.00% <ø> (ø) 15.00 <0.00> (ø)
...in/java/io/opentelemetry/sdk/OpenTelemetrySdk.java 100.00% <ø> (ø) 3.00 <0.00> (ø)
.../java/io/opentelemetry/sdk/internal/TestClock.java 100.00% <ø> (ø) 8.00 <0.00> (ø)
...telemetry/sdk/resources/EnvAutodetectResource.java 100.00% <ø> (ø) 4.00 <0.00> (ø)
...io/opentelemetry/sdk/metrics/MeterSdkProvider.java 100.00% <ø> (ø) 5.00 <0.00> (ø)
.../io/opentelemetry/sdk/metrics/data/MetricData.java 100.00% <ø> (ø) 2.00 <0.00> (ø)
...metry/sdk/metrics/export/IntervalMetricReader.java 87.69% <ø> (ø) 4.00 <0.00> (ø)
...o/opentelemetry/sdk/metrics/view/Aggregations.java 76.78% <ø> (ø) 4.00 <0.00> (ø)
.../io/opentelemetry/sdk/trace/TracerSdkProvider.java 100.00% <ø> (ø) 11.00 <0.00> (ø)
...io/opentelemetry/sdk/trace/config/TraceConfig.java 95.00% <ø> (ø) 6.00 <0.00> (ø)
... and 14 more

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 a4cd21f...ce3ee9e. Read the comment docs.

@jkwatson
Copy link
Contributor

It looks like this PR now includes commits from previously merged PRs. I think those need to be removed from this PR.

@euniceek euniceek marked this pull request as draft September 17, 2020 19:18
@euniceek euniceek closed this Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anuraaga anuraaga anuraaga left review comments

@arminru arminru arminru approved these changes

@thisthat thisthat thisthat approved these changes

@jkwatson jkwatson jkwatson approved these changes

@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.
6 participants
@euniceek @Oberon00 @thisthat @jkwatson @anuraaga @arminru