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

Add SummaryDataPoint support to Metrics proto (opentelemetry/opentele… #227

Merged
merged 4 commits into from
Nov 4, 2020
Merged

Add SummaryDataPoint support to Metrics proto (opentelemetry/opentele… #227

merged 4 commits into from
Nov 4, 2020

Conversation

gcacace
Copy link
Contributor

@gcacace gcacace commented Oct 28, 2020

…metry-specification#1146)

  • Add IntSummary and DoubleSummary to data
  • Add IntSummaryDataPoint and DoubleSummaryDataPoint
  • Comments

@gcacace gcacace requested review from a team October 28, 2020 12:57
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 28, 2020 •
edited
Loading

CLA Check
The committers are authorized under a signed CLA.

This was referenced Oct 28, 2020
Closed
Closed
@gcacace gcacace force-pushed the master branch 2 times, most recently from 6e615f6 to 30e6ce5 Compare October 28, 2020 14:32
@gcacace
Copy link
Contributor Author

gcacace commented Oct 28, 2020

Please consider the main use case for IntSummary and DoubleSummary is for transport purposes (not aggregation). In this specific case I've removed AggregationTemporality, addressing concerns expressed in #199.

I've added min and max.

Not sure if we need to support both IntSummary and DoubleSummary, or the Double version is enough (I've included both for model completeness).

Not sure if we need to support exemplars.

@gcacace gcacace force-pushed the master branch 4 times, most recently from f469c95 to 758d2c1 Compare October 30, 2020 09:18
@gcacace
Add SummaryDataPoint support to Metrics proto (opentelemetry/opentele…
ecf409d 
…metry-specification#1146)

* Add DoubleSummary to data
* Add DoubleSummaryDataPoint
* Comments
@jmacd jmacd mentioned this pull request Oct 30, 2020
Closed
@gcacace
Copy link
Contributor Author

gcacace commented Nov 2, 2020

Can someone with merge permissions push and close this change please? When it is going to be released in Maven?

@bogdandrutu
Copy link
Member

We don't release this repo to maven (that is a Java specific repo, if I am not wrong). The other projects consume this repo as git submodule and generates their code.

@bogdandrutu
Copy link
Member

@open-telemetry/specs-metrics-approvers please take a look.

rakyll
rakyll reviewed Nov 2, 2020
View reviewed changes
Copy link

@rakyll rakyll left a comment

Choose a reason for hiding this comment

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

I'm not familiar with the OTLP yet to give a more comprehensive review but I am leaving two high level questions.

opentelemetry/proto/metrics/v1/metrics.proto Show resolved Hide resolved
opentelemetry/proto/metrics/v1/metrics.proto Show resolved Hide resolved
@gcacace
Copy link
Contributor Author

gcacace commented Nov 2, 2020

We don't release this repo to maven (that is a Java specific repo, if I am not wrong). The other projects consume this repo as git submodule and generates their code.

Who should I ask in order to have a released version on this code in Maven then? https://mvnrepository.com/artifact/io.opentelemetry/opentelemetry-proto

@JasonXZLiu JasonXZLiu mentioned this pull request Nov 2, 2020
Merged
@bogdandrutu
Copy link
Member

@gcacace that is generated by the opentelemetry-java repo. So once this is merged, and the otel java repo resyncs will export this to maven.

MrAlias
MrAlias approved these changes Nov 3, 2020
View reviewed changes
opentelemetry/proto/metrics/v1/metrics.proto Outdated Show resolved Hide resolved
opentelemetry/proto/metrics/v1/metrics.proto Outdated Show resolved Hide resolved
@gcacace
Copy link
Contributor Author

gcacace commented Nov 4, 2020 •
edited
Loading

How can I get more traction on closing out this PR? It is currently blocking some work on our side.

@bogdandrutu
Copy link
Member

@gcacace i think the author needs to respond and fix some of the comments.

@bogdandrutu bogdandrutu closed this Nov 4, 2020
@bogdandrutu bogdandrutu reopened this Nov 4, 2020
@gcacace
Copy link
Contributor Author

gcacace commented Nov 4, 2020

All comments addressed.

tigrannajaryan
tigrannajaryan approved these changes Nov 4, 2020
View reviewed changes
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM

@JasonXZLiu
Copy link
Member

JasonXZLiu commented Nov 4, 2020 •
edited
Loading

@tigrannajaryan @bogdandrutu Would it be possible to make a release with these changes right after merging this PR? We have some blocked PR's waiting for this datapoint to be added to the proto. I don't think I have the access to make a release.

@bogdandrutu bogdandrutu merged commit 8ab21e9 into open-telemetry:master Nov 4, 2020
@bogdandrutu
Copy link
Member

Don't need a release just update the submodule to point to the merged commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JasonXZLiu JasonXZLiu JasonXZLiu approved these changes

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@rakyll rakyll rakyll left review comments

@MrAlias MrAlias MrAlias approved these changes

@tigrannajaryan tigrannajaryan tigrannajaryan approved these changes

@jmacd jmacd jmacd approved these changes

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

Successfully merging this pull request may close these issues.
7 participants
@gcacace @bogdandrutu @JasonXZLiu @rakyll @jmacd @tigrannajaryan @MrAlias