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(mongodb): collect mongodb4 metrics #1170

Merged
merged 68 commits into from
May 23, 2023

Conversation

osherv
Copy link
Member

@osherv osherv commented Sep 14, 2022

Which problem is this PR solving?

Short description of the changes

Fixes #1157

Added metrics collection for MongoDB >= 4, according to:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/database-metrics.md
This is the first of many contributions planned under metrics collections at the instrumentation level, so I'll be really happy to get as much as comments as we can on this :)
Thanks!

Checklist

  • Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable.

@osherv osherv changed the title Feat/add mongodb metrics feat(mongodb): add mongodb metrics Sep 14, 2022
@osherv osherv changed the title feat(mongodb): add mongodb metrics feat(mongodb): collect mongodb metrics Sep 14, 2022
@codecov
Copy link

codecov bot commented Sep 14, 2022

Codecov Report

Merging #1170 (2fd6bd2) into main (418b6f6) will decrease coverage by 0.76%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1170      +/-   ##
==========================================
- Coverage   96.13%   95.38%   -0.76%     
==========================================
  Files          14       16       +2     
  Lines         906      997      +91     
  Branches      197      206       +9     
==========================================
+ Hits          871      951      +80     
- Misses         35       46      +11     

see 2 files with indirect coverage changes

@osherv osherv marked this pull request as ready for review September 19, 2022 15:12
@osherv osherv requested a review from a team September 19, 2022 15:12
@osherv osherv changed the title feat(mongodb): collect mongodb metrics feat(mongodb): collect mongodb4 metrics Sep 19, 2022
@vmarchaud
Copy link
Member

Do you want to wait for open-telemetry/opentelemetry-js#3267 to merge this ?

@osherv
Copy link
Member Author

osherv commented Sep 20, 2022

@vmarchaud Yea, probably, thanks :)

@vmarchaud vmarchaud marked this pull request as draft September 20, 2022 13:23
@vmarchaud
Copy link
Member

Converting to draft in the mean time :)

@haddasbronfman
Copy link
Member

@legendecas @dyladan Hi. I'm looking for reviewers who are familiar with metrics. Can you please review this PR?

Copy link
Member

@legendecas legendecas 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 mongodb. The metrics instrumentation looks good to me.

const inMemoryMetricsExporter = new InMemoryMetricExporter(
AggregationTemporality.CUMULATIVE
);
const metricReader = new PeriodicExportingMetricReader({
Copy link
Member

Choose a reason for hiding this comment

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

Nit: IIUC, a pulling metric reader might be better suited for testing usage since it can pull metrics proactively instead of awaiting timeouts.

@pichlermarc
Copy link
Member

Looks like TAV is failing on line 137 in the v4 metrics tests, the idle state is now exporting 2. 🤔

@haddasbronfman haddasbronfman merged commit 988e1f8 into open-telemetry:main May 23, 2023
@dyladan dyladan mentioned this pull request May 23, 2023
sberz added a commit to sberz/opentelemetry-js-contrib that referenced this pull request Aug 18, 2023
The type `V4Connection` re-introduced in open-telemetry#1170 is missing any type
definitions or imports for `Document`, breaking the build with
typecricpt strict mode.
`V4Connection` in `types.ts` is not referenced anywhere (the type
definition in `internal-types.ts` is used. So lets just drop this type
definition.

BREAKING CHANGE: removes the broken exported type `V4Connection`.
Fixes open-telemetry#1639
sberz added a commit to sberz/opentelemetry-js-contrib that referenced this pull request Aug 18, 2023
The type `V4Connection` re-introduced in open-telemetry#1170 is missing any type
definitions or imports for `Document`, breaking the build with
Typescript strict mode.
`V4Connection` in `types.ts` is not referenced anywhere (the type
definition in `internal-types.ts` is used. So lets just drop this type
definition.

BREAKING CHANGE: removes the broken exported type `V4Connection`.
Fixes open-telemetry#1639
sberz added a commit to sberz/opentelemetry-js-contrib that referenced this pull request Aug 18, 2023
The type `V4Connection` re-introduced in open-telemetry#1170 is missing any type
definitions or imports for `Document`, breaking the build with
Typescript strict mode.
`V4Connection` in `types.ts` is not referenced anywhere (the type
definition in `internal-types.ts` is used. So lets just drop this type
definition.

BREAKING CHANGE: removes the broken exported type `V4Connection`.
Fixes open-telemetry#1639

Signed-off-by: Simon Berz <simon@berz.me>
blumamir pushed a commit that referenced this pull request Aug 26, 2023
The type `V4Connection` re-introduced in #1170 is missing any type
definitions or imports for `Document`, breaking the build with
Typescript strict mode.
`V4Connection` in `types.ts` is not referenced anywhere (the type
definition in `internal-types.ts` is used. So lets just drop this type
definition.

BREAKING CHANGE: removes the broken exported type `V4Connection`.
Fixes #1639

Signed-off-by: Simon Berz <simon@berz.me>
@dyladan dyladan mentioned this pull request Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metrics instrumentaion MongoDB
6 participants