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

Metric aggregators #1144

Closed
obecny opened this issue Jun 4, 2020 · 8 comments
Closed

Metric aggregators #1144

obecny opened this issue Jun 4, 2020 · 8 comments
Labels
Discussion Issue or PR that needs/is extended discussion. feature-request

Comments

@obecny
Copy link
Member

obecny commented Jun 4, 2020

Currently our aggregators doesn't follow the naming conventions from spec
The spec describes 2 aggregators:

  • sum (Counter, UpDownCounter, SumObserver, UpDownSumObserver)
  • minMaxSumCount (ValueRecorder, ValueObserver)
    and we have 3 (countersum, Observer, ValueRecorderExact)

We also have some other differences
Our current Observer is in fact SumObserver
but according to this #1113 I have just changed this to ValueObserver.

We also have this task
#1114
about implementing a SumObserver which in fact is our current observer under valueobserver name

Anyway I want to clean this up
Rename then ValueObserver into SumObserver.
Close the task about creating new SumObserver and create a new task to create a ValueObserver. And then create / rename the aggregators and use them correctly to new spec. I think it would make sense to make it in one PR, unless you have an idea how this could be splited
@obecny obecny added feature-request Discussion Issue or PR that needs/is extended discussion. labels Jun 4, 2020
@mayurkale22
Copy link
Member

Currently our aggregators doesn't follow the naming conventions from spec

Are you suggesting something like this?
ValueRecorderExactAggregator => MinMaxSumCountAggregator
CounterSumAggregator => SumAggregator

@obecny
Copy link
Member Author

obecny commented Jun 4, 2020

Currently our aggregators doesn't follow the naming conventions from spec

Are you suggesting something like this?
ValueRecorderExactAggregator => MinMaxSumCountAggregator
CounterSumAggregator => SumAggregator

exactly

@mayurkale22
Copy link
Member

Agreed.

@obecny
Copy link
Member Author

obecny commented Jun 4, 2020

or I can keep renaming observer to valueobserver and then make necessary changes so that the valueobserver will be the same as in spec, and then #1114 will remain as it is, maybe this makes more sense ?

@obecny
Copy link
Member Author

obecny commented Jun 4, 2020

new metric kinds here:
#1145

@obecny
Copy link
Member Author

obecny commented Jun 5, 2020

@dyladan @mayurkale22 from what I see the spec says nothing about LastValue which is weird or there is a bug. Anyway if we implement all as in spec we don't have possibility of having the LastValue Observer. Because we don't have yet SumObserver and UpDownSumObserver and I just renamed old observer into ValueObserver I will not yet change the aggregator for it from LastValue to SumObserver. If I think about this more I have a feeling that something is missing here.

@obecny
Copy link
Member Author

obecny commented Jun 5, 2020

I have raised an issue in spec -> open-telemetry/opentelemetry-specification#636

@legendecas
Copy link
Member

Closing as the issue is no longer applicable.

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this issue Dec 15, 2023
@carlpett
fix(readme): Correct urls to npm (open-telemetry#1144)
d8767a9 
Apparent copy/paste error pointed to instrumentation-dns instead of
instrumentation-express

Co-authored-by: Amir Blum <amir@aspecto.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Discussion Issue or PR that needs/is extended discussion. feature-request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@obecny @mayurkale22 @legendecas