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

feat: Metrics SDK - aggregator, batcher, controller #738

Merged
merged 7 commits into from
Feb 18, 2020

Conversation

mayurkale22
Copy link
Member

@mayurkale22 mayurkale22 commented Jan 28, 2020

Short description of the changes

TODO:

Note:

  • Submitting PR to get early feedback on API, Usage etc.
  • I am more than happy to split this work in smaller PRs. For now wanted to give a complete sense of implementation and to see how all things fit toghther.

@mayurkale22 mayurkale22 added Discussion Issue or PR that needs/is extended discussion. size/XL labels Jan 28, 2020
@codecov-io
Copy link

codecov-io commented Jan 28, 2020

Codecov Report

Merging #738 into master will decrease coverage by 12.12%.
The diff coverage is 77.77%.

@@             Coverage Diff             @@
##           master     #738       +/-   ##
===========================================
- Coverage   92.55%   80.42%   -12.13%     
===========================================
  Files         240       99      -141     
  Lines       10819     3776     -7043     
  Branches      990      333      -657     
===========================================
- Hits        10013     3037     -6976     
+ Misses        806      739       -67
Impacted Files Coverage Δ
...metry-core/src/trace/instrumentation/BasePlugin.ts 86.84% <ø> (ø) ⬆️
...opentelemetry-node/test/NodeTracerProvider.test.ts 100% <ø> (ø)
...ackages/opentelemetry-api/src/metrics/NoopMeter.ts 81.13% <ø> (ø) ⬆️
packages/opentelemetry-api/src/metrics/Metric.ts 100% <ø> (ø) ⬆️
...pentelemetry-core/test/internal/validators.test.ts 0% <0%> (-100%) ⬇️
...ages/opentelemetry-core/src/internal/validators.ts 100% <100%> (ø) ⬆️
...try-node/test/instrumentation/PluginLoader.test.ts 100% <100%> (ø) ⬆️
...telemetry-node/src/instrumentation/PluginLoader.ts 92.53% <50%> (-1.12%) ⬇️
...entelemetry-core/test/common/ConsoleLogger.test.ts 0% <0%> (-100%) ⬇️
...try-core/test/trace/fixtures/test-package/index.js 0% <0%> (-100%) ⬇️
... and 195 more

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

This is really an awesome start, thanks for this. It looks like a pretty big improvement on the old version.

packages/opentelemetry-metrics/src/export/Aggregator.ts Outdated Show resolved Hide resolved
monotonic: boolean,
valueType: types.ValueType,
logger: types.Logger,
aggregator: Aggregator
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the counter always use the sum aggregator?

Copy link
Member

Choose a reason for hiding this comment

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

I know we talked about this in the SIG meeting a couple weeks ago, but I don't remember the resolution.

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed offline, hardcoding will certainly simplify the things and will be less buggy (and easy to read). Also, you're right (99.9%) that counter should always have CounterSumAggregator. But I haven't seen any hardcoded stuff in specs or Go/Python implementations. Aggregators are always coming from Batcher(aggregatorFor - function). I was thinking to keep it as it is and open an issue to consider doing it in the future, once specs is completely flushed out.

@mayurkale22
Copy link
Member Author

@open-telemetry/javascript-approvers We need reviews for this one, please take a look when you get a chance.

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Overall this looks good, just have a few unresolved questions from before.

packages/opentelemetry-metrics/src/export/Aggregator.ts Outdated Show resolved Hide resolved
monotonic: boolean,
valueType: types.ValueType,
logger: types.Logger,
aggregator: Aggregator
Copy link
Member

Choose a reason for hiding this comment

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

I know we talked about this in the SIG meeting a couple weeks ago, but I don't remember the resolution.

@mayurkale22 mayurkale22 added this to the Alpha v0.4 milestone Feb 11, 2020
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Think I'm happy with this as a good place we can iterate off of. Thanks for doing this :)

@dyladan dyladan mentioned this pull request Feb 12, 2020
46 tasks
@mayurkale22 mayurkale22 modified the milestones: Alpha v0.4, Beta Feb 12, 2020
Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

lgtm

@dyladan
Copy link
Member

dyladan commented Feb 18, 2020

I think this is ready to merge if you're happy with it @mayurkale22

@dyladan dyladan added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Feb 18, 2020
@mayurkale22 mayurkale22 merged commit d35ac4f into open-telemetry:master Feb 18, 2020
@mayurkale22 mayurkale22 deleted the metrics_sdk branch February 18, 2020 19:21
@obecny
Copy link
Member

obecny commented Feb 25, 2020

guys changes here caused the example for Prometheus is not working anymore

@mayurkale22
Copy link
Member Author

Thanks for reporting this, will try to fix it today.

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
…en-telemetry#738)

* chore: calc propagation fields count before inject

* chore(aws-sdk): lint:fix

* chore(aws-sdk): cover MessageAttributes with tests

Co-authored-by: Amir Blum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issue or PR that needs/is extended discussion. Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants