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

Consider how to deal with already registered metric #474

Closed
mayurkale22 opened this issue Nov 1, 2019 · 2 comments
Closed

Consider how to deal with already registered metric #474

mayurkale22 opened this issue Nov 1, 2019 · 2 comments
Labels
question User is asking a question not related to a new feature or bug

Comments

@mayurkale22
Copy link
Member

Summary:

A given metric is only meant to be registered once, this issue is to decide how to handle duplicate or already registered metric (Currently not permitted and cause an error with Prometheus).

Possible Solutions:

  • Option 1: Replace the old registered metric by the new: This seems to be problematic, especially when you use the same name but different type or unit. This might create a problem in backend.

  • Option 2: Throw an error: This is not really advocated by the specs.

  • Option 3: Log a warning and skip duplicate metric: This might confuse the end users.

Suggest other options?
@mayurkale22 mayurkale22 added the question User is asking a question not related to a new feature or bug label Nov 1, 2019
mayurkale22 added a commit to mayurkale22/opentelemetry-node that referenced this issue Nov 1, 2019
@mayurkale22
fix: link open-telemetry#474 isssue
f3709ea
mayurkale22 added a commit that referenced this issue Nov 1, 2019
@mayurkale22
feat(metrics): add registerMetric and getMetrics (#437)
173cab2 
* feat(metrics): add registerMetric and getMetric functionality

* fix: getTimeSeries and add more tests

* fix: minor

* fix: add JSDoc

* fix: minor

* fix: remove

* fix: replace String -> string

* fix: avoid casting

* fix: use GAUGE_DOUBLE and COUNTER_DOUBLE type

* fix: add ValueType to indicate type of the metric

* fix: move ValueType.DOUBLE to DEFAULT_METRIC_OPTIONS

* fix: use Number.isInteger

* fix: log an error instead of throw error

* fix: add more test and @todo comment

* fix: link #474 isssue
@dyladan
Copy link
Member

dyladan commented Nov 1, 2019

It is my opinion that Option 3 is the only currently viable option as option 1 may create problems with different backends (maybe this is handled by individual exporters?) and option 2 is not compatible with current spec (incorrect usage should not throw).

Option 3 may be confusing at first (generally I would expect it to behave like other Set-like APIs which overwrite rather than ignore if you try to set the same thing twice), but it is an easy to understand difference if documented properly.

@mayurkale22
Copy link
Member Author

Looks like we have an agreement here, closing this issue now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
question User is asking a question not related to a new feature or bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@dyladan @mayurkale22