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

Add incubator module with utilities for mutating SpanData. #1625

Merged
merged 3 commits into from
Sep 24, 2020

Conversation

anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Sep 8, 2020

Sorry for the huge delay on this.

For #1321

@codecov
Copy link

codecov bot commented Sep 8, 2020

Codecov Report

Merging #1625 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1625      +/-   ##
============================================
- Coverage     85.45%   85.41%   -0.04%     
  Complexity     1382     1382              
============================================
  Files           164      164              
  Lines          5418     5418              
  Branches        556      556              
============================================
- Hits           4630     4628       -2     
- Misses          588      589       +1     
- Partials        200      201       +1     
Impacted Files Coverage Δ Complexity Δ
...try/sdk/metrics/aggregator/LongMinMaxSumCount.java 96.07% <0.00%> (-3.93%) 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 3d72a7d...7f4e711. Read the comment docs.

// type.
@Immutable
@AutoValue
public abstract class SpanData implements io.opentelemetry.sdk.trace.data.SpanData {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we name this something else, so we don't have the name collision, requiring using FQNs for hte interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why I missed this review, sorry! I could have sworn I added a comment in description about it but didn't - I agree, but couldn't come up with a better name :( Any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

BuildableSpanData? SpanDataExtender? BuiltSpanData?

Maybe just SpanDataBuilder for now, since it's really a temporary move to see if this is the best approach?

@jkwatson
Copy link
Contributor

jkwatson commented Sep 8, 2020

Thanks! Just one small question about naming the one with the builder.

@jkwatson
Copy link
Contributor

@anuraaga any updates on the naming here?

@carlosalberto
Copy link
Contributor

Is this really urgent?

@jkwatson
Copy link
Contributor

Is this really urgent?

It's not, but I'd rather not have open PRs lying around, even if they aren't technically needed for GA.

@anuraaga
Copy link
Contributor Author

@jkwatson Yup sorry for the slowness, finally renamed.

@jkwatson jkwatson merged commit f63c84f into open-telemetry:master Sep 24, 2020
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.

3 participants