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

Combined Metrics API updates for v0.3 (Add Observer instrument, remove Gauge instrument, etc.) #430

Merged
merged 57 commits into from
Feb 14, 2020

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Jan 29, 2020

This specifies the changes described in these OTEPs:
open-telemetry/oteps#80
open-telemetry/oteps#72

@jmacd jmacd mentioned this pull request Jan 30, 2020
@jmacd
Copy link
Contributor Author

jmacd commented Jan 30, 2020

I've added a couple pages of examples.

specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Show resolved Hide resolved
specification/api-metrics.md Outdated Show resolved Hide resolved
@jmacd jmacd changed the title WIP: Add Observer instrument, remove Gauge instrument Add Observer instrument, remove Gauge instrument Feb 3, 2020
Copy link
Contributor Author

@jmacd jmacd 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 @yurishkuro. This is excellent feedback.

specification/api-metrics.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

I am interested in feedback on the discussion points here from the community.

specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Show resolved Hide resolved
specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Show resolved Hide resolved
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Some small concerns, a lot of nits and requests to file issues to discuss more.

specification/api-metrics.md Show resolved Hide resolved
specification/api-metrics.md Show resolved Hide resolved
specification/api-metrics.md Show resolved Hide resolved
specification/api-metrics.md Show resolved Hide resolved
specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Outdated Show resolved Hide resolved
specification/api-metrics.md Show resolved Hide resolved
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Very nice document.

specification/api-metrics.md Show resolved Hide resolved
@dyladan
Copy link
Member

dyladan commented Feb 13, 2020

suggest renaming this PR because I have been not watching changes assuming the title made it self-evident what has been going on here. This looks way bigger than the title suggests.

@jmacd jmacd changed the title Add Observer instrument, remove Gauge instrument Combined Metrics API updates for v0.3 (Add Observer instrument, remove Gauge instrument, etc.) Feb 13, 2020
@jmacd
Copy link
Contributor Author

jmacd commented Feb 13, 2020

@dyladan You're right. I've added more detail in the title.

@jmacd
Copy link
Contributor Author

jmacd commented Feb 13, 2020

@bogdandrutu Please take another look.

@jmacd
Copy link
Contributor Author

jmacd commented Feb 13, 2020

@dyladan I looks big, you're right. There is a lot of philosophical justification here for removing Gauge and adding Observer. That's really just ab out all that's happened here.

@bogdandrutu bogdandrutu merged commit 4e7cd5a into open-telemetry:master Feb 14, 2020
SergeyKanzhelev pushed a commit to SergeyKanzhelev/opentelemetry-specification that referenced this pull request Feb 18, 2020
…e Gauge instrument, etc.) (open-telemetry#430)

* Gauge/Observer rewrite pt 1

* Checkpoint

* Checkpoint

* Optional features restored

* Update selection process

* Update TOC

* Reword intro para on standard impl

* Add detail on label set

* Add examples

* Misspellings

* Rewrite the introductory material

* Add a section on time

* Clarify 'standard implementation' and 'default interpretation'

* Explain aggregations; Discourage timestamp use

* Reword the metric event format

* Update Counter and Measure

* Update Observer (partial)

* Remove options, discuss views API

* Fixes

* Fixes

* Fixes

* Rename BoundTimer

* Typos and suggestions

* Reword global Meter

* Rearrange early paragraphs

* Use _run time_ and _simultaneous_

* Rearrange early paragraphs (again)

* Define _run time_

* These

* Iterate

* Remove a TODO

* More on global/named meters

* Difference, Capture

* Timer wording

* Explicit timestamp

* Rewrite simultaneous

* Example: active requests

* Synchronously

* Wording

* Example using correlation context

* Detail on WithKeys

* Counter is a special case

* Counter fixes; typo

* Typo

* Split the leading example into Counter and Measure cases

* Remove poor example

* Update TOC

* Add note about Observer and Context

* Add note about context-freedom in Observer

* Run MDL

* WithRecommendedKeys

* Typo

* Address most of Bogdan's feedback

* Typo

Co-authored-by: Bogdan Drutu <lazy@splunk.com>
@arminru arminru added the spec:metrics Related to the specification/metrics directory label Feb 18, 2020
@jmacd jmacd deleted the jmacd/api-metrics-03 branch April 14, 2020 04:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.