Skip to content

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

Merged
bogdandrutu merged 4 commits intoopen-telemetry:masterfrom
gcacace:master
Nov 4, 2020
Merged

Add SummaryDataPoint support to Metrics proto (opentelemetry/opentele…#227
bogdandrutu merged 4 commits intoopen-telemetry:masterfrom
gcacace:master

Conversation

@gcacace
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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.

JasonXZLiu
JasonXZLiu reviewed Oct 28, 2020
View reviewed changes
bogdandrutu
bogdandrutu reviewed Oct 29, 2020
View reviewed changes
@gcacace gcacace force-pushed the master branch 4 times, most recently from f469c95 to 758d2c1 Compare October 30, 2020 09:18
gcacace
gcacace commented Oct 30, 2020
View reviewed changes
@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
Copy Markdown
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
Copy Markdown
Member

bogdandrutu 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.

@bogdandrutu
Copy link
Copy Markdown
Member

bogdandrutu commented Nov 2, 2020

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

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

@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.

@gcacace
Copy link
Copy Markdown
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
Copy Markdown
Member

bogdandrutu commented Nov 2, 2020

@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
tigrannajaryan
tigrannajaryan reviewed Nov 3, 2020
View reviewed changes
@gcacace
Copy link
Copy Markdown
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
Copy Markdown
Member

bogdandrutu commented Nov 4, 2020

@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
Copy Markdown
Contributor Author

gcacace commented Nov 4, 2020

All comments addressed.

tigrannajaryan
tigrannajaryan approved these changes Nov 4, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

bogdandrutu commented Nov 4, 2020

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

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@jmacd jmacd jmacd approved these changes

@tigrannajaryan tigrannajaryan tigrannajaryan approved these changes

@MrAlias MrAlias MrAlias approved these changes

+2 more reviewers

@rakyll rakyll rakyll left review comments

@JasonXZLiu JasonXZLiu JasonXZLiu approved these changes

Reviewers whose approvals may not affect merge requirements

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