feat(instrumentation): add new setMeterInstruments method #3267

osherv · 2022-09-19T12:51:08Z

This issue allows the instrumentations to only implement the setMeterInstruments when collecting metrics.

Before:

export class MongoDBInstrumentation extends InstrumentationBase {
  private _connectionsUsage!: UpDownCounter;

  constructor(protected override _config: MongoDBInstrumentationConfig = {}) {
    super('@opentelemetry/instrumentation-mongodb', VERSION, _config);
    this._setMetricInstruments();
  }

  override setMeterProvider(meterProvider: MeterProvider) {
    super.setMeterProvider(meterProvider);
    this._setMetricInstruments();
  }

  private _setMetricInstruments() {
    this._connectionsUsage = this.meter.createUpDownCounter(
      'active_connections',
      {
        description:
          'The number of connections that are currently in state described by the state attribute.',
      }
    );
  }

After:

export class MongoDBInstrumentation extends InstrumentationBase {
  private _connectionsUsage!: UpDownCounter;

  private _setMetricInstruments() {
    this._connectionsUsage = this.meter.createUpDownCounter(
      'active_connections',
      {
        description:
          'The number of connections that are currently in state described by the state attribute.',
      }
    );
  }

… instruments every meter provider update.

codecov · 2022-09-19T12:55:02Z

Codecov Report

Merging #3267 (7c67a60) into main (e315f53) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3267      +/-   ##
==========================================
- Coverage   93.26%   93.23%   -0.03%     
==========================================
  Files         247      247              
  Lines        7348     7348              
  Branches     1512     1512              
==========================================
- Hits         6853     6851       -2     
- Misses        495      497       +2

Impacted Files	Coverage Δ
...ges/opentelemetry-instrumentation-http/src/http.ts	`94.71% <100.00%> (-0.07%)`	⬇️
...entelemetry-instrumentation/src/instrumentation.ts	`80.00% <100.00%> (+10.76%)`	⬆️
...ckages/opentelemetry-sdk-trace-web/karma.worker.js	`0.00% <0.00%> (-100.00%)`	⬇️

experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts

…mentation.ts Co-authored-by: Chengzhong Wu <[email protected]>

osherv · 2022-09-20T07:19:42Z

@legendecas agreed with your comments, thanks :)

legendecas

Generally LGTM! Thank you for working on this.

experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts

….com:epsagon/opentelemetry-js into feat/add-update-metric-instruments-interface

osherv · 2022-11-06T11:19:39Z

Hey :) @legendecas
Sorry for the delay but added tests now.

legendecas

LGTM % nits.

experimental/packages/opentelemetry-instrumentation-http/src/http.ts

experimental/CHANGELOG.md

legendecas

LGTM

dyladan · 2022-11-08T16:30:41Z

experimental/packages/opentelemetry-instrumentation/package.json

@@ -76,6 +76,7 @@
    "@opentelemetry/api": "^1.2.0"
  },
  "devDependencies": {
+    "@opentelemetry/sdk-metrics": "^0.32.0",


I think this is already otudated

legendecas

Still LGTM

legendecas · 2022-11-11T02:35:04Z

experimental/packages/opentelemetry-instrumentation/package.json

@@ -76,6 +76,7 @@
    "@opentelemetry/api": "^1.3.0"
  },
  "devDependencies": {
+    "@opentelemetry/sdk-metrics": "^1.8.0",


I believe this needs to follow our dev dependency conventions to set a specific version instead of version ranges. However, I can not edit the PR before merging.

Suggested change

"@opentelemetry/sdk-metrics": "^1.8.0",

"@opentelemetry/sdk-metrics": "1.8.0",

Tips: when you create the PR, if you are ok with it, it would be more convenient for us to adopt those miscellaneous snippets before merging if you can check this option.

Thanks! it will be enabled from my next pr (currently I'm under my company fork so I can't set this option).

@osherv could you do the change and rebase so we can merge this ?

ready for merge :)

I still didn't find the caret being removed. Would you mind verifying if your commits are pushed?

Has this been updated?

experimental/CHANGELOG.md

legendecas · 2022-11-15T17:42:45Z

:( it looks like we need another update with the main branch.

* feat(mongodb): started working on mongodb connection pool instrumentations * feat(mongodb): started working on mongodb connection pool instrumentations * feat(mongodb): started working on mongodb connection pool instrumentations * feat(mongodb): started working on mongodb connection pool instrumentations * feat(mongodb): added connection metrics mongodb4 before tests * feat(mongodb): started working on tests * feat(mongodb): started working on tests * feat(mongodb): started working on tests * feat(mongodb): started working on tests * feat(mongodb): started working on tests * feat(mongodb): added mongodb4 metric tests * feat(mongodb): added mongodb4 metric tests * feat(mongodb): added mongodb4 metric tests * feat(mongodb): started working on mongodb3 metrics * feat(mongodb): removed <4 supprot * fix(mongo4): mongodb v4 instrumentation. fix idle/used counting * fix(mongo4): lint * fix(mongo4): use _updateMetricInstruments() as described in open-telemetry#3267 * fix(mongo4): lint * fix(mongo4): change deps * fix(mongo4): change deps 2 * fix(mongo4): revert deps * fix(mongo4): fixes * fix(mongo4): lint * fix(mongo4): lexport type * fix(mongo4): fixes * fix(mongo4): fixes * fix(mongo4): fixes: remote redundant from types, remove unneeded comments * fix(mongo4): add connection_string * fix(mongo4): add undefined instead of connection_string for for mongo v3 * Update plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts Co-authored-by: Marc Pichler <[email protected]> * Update plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts Co-authored-by: Marc Pichler <[email protected]> * fix(mongo4): move V4Connect and V4Session to internal-types * fix(mongo): lint * fix(mongo): align metric description with semconv * Update plugins/node/opentelemetry-instrumentation-mongodb/test/mongodb-v4.metrics.test.ts Co-authored-by: Marc Pichler <[email protected]> * fix(mongo): fix sessions.length to work also for versions 4.1 - 4.11 * fix(mongo): try to fix TAV for node 16 & 18 that doesn't work for mongo 3.6.2 --------- Co-authored-by: haddasbronfman <[email protected]> Co-authored-by: Haddas Bronfman <[email protected]> Co-authored-by: Marc Pichler <[email protected]>

feat(instrumentation): add new protected method that update the meter…

2fafb4c

… instruments every meter provider update.

osherv requested a review from a team September 19, 2022 12:51

feat(instrumentation): fixed markdown lint

5188bb9

vmarchaud mentioned this pull request Sep 19, 2022

feat(mongodb): collect mongodb4 metrics open-telemetry/opentelemetry-js-contrib#1170

Merged

1 task

legendecas reviewed Sep 20, 2022

View reviewed changes

experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts Outdated Show resolved Hide resolved

experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts Outdated Show resolved Hide resolved

osherv and others added 3 commits September 20, 2022 10:18

Update experimental/packages/opentelemetry-instrumentation/src/instru…

b92bb79

…mentation.ts Co-authored-by: Chengzhong Wu <[email protected]>

Update experimental/packages/opentelemetry-instrumentation/src/instru…

a1d9119

…mentation.ts Co-authored-by: Chengzhong Wu <[email protected]>

Merge branch 'main' into feat/add-update-metric-instruments-interface

d5b3ddc

osherv added 2 commits September 20, 2022 13:06

feat(instrumentation): renamed function

77c9374

feat(instrumentation): adapted http instrumentation to the new api

75af723

legendecas reviewed Sep 20, 2022

View reviewed changes

experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts Show resolved Hide resolved

legendecas mentioned this pull request Nov 4, 2022

feat(mysql): Metrics for mysql open-telemetry/opentelemetry-js-contrib#1220

Merged

osherv and others added 4 commits November 6, 2022 11:59

Merge branch 'main' into feat/add-update-metric-instruments-interface

210b77b

feat(instrumentation): added tests

f768af1

Merge branch 'feat/add-update-metric-instruments-interface' of github…

d6952f8

….com:epsagon/opentelemetry-js into feat/add-update-metric-instruments-interface

feat(instrumentation): resolved conlicts

7328f00

legendecas reviewed Nov 7, 2022

View reviewed changes

experimental/packages/opentelemetry-instrumentation-http/src/http.ts Outdated Show resolved Hide resolved

experimental/CHANGELOG.md Outdated Show resolved Hide resolved

feat(instrumentation): fixed legendcas CR

c6c2be9

legendecas approved these changes Nov 7, 2022

View reviewed changes

Merge branch 'main' into feat/add-update-metric-instruments-interface

f65c81e

pichlermarc approved these changes Nov 8, 2022

View reviewed changes

dyladan reviewed Nov 8, 2022

View reviewed changes

osherv and others added 3 commits November 9, 2022 12:19

Merge branch 'main' into feat/add-update-metric-instruments-interface

244cd98

Merge branch 'main' into feat/add-update-metric-instruments-interface

8f70a7f

feat(instrumentation): fixed dyladan's CR

fe7880c

legendecas approved these changes Nov 11, 2022

View reviewed changes

legendecas reviewed Nov 11, 2022

View reviewed changes

Merge branch 'main' into feat/add-update-metric-instruments-interface

9df8e43

vmarchaud reviewed Nov 13, 2022

View reviewed changes

experimental/CHANGELOG.md Outdated Show resolved Hide resolved

osherv and others added 3 commits November 14, 2022 16:25

Merge branch 'main' into feat/add-update-metric-instruments-interface

3bfca7f

feat(instrumentation): fixed vmarchaud's CR

0ef2766

Merge branch 'main' into feat/add-update-metric-instruments-interface

ca02353

osherv added 3 commits November 16, 2022 11:20

Merge branch 'main' into feat/add-update-metric-instruments-interface

f31f953

Merge branch 'main' into feat/add-update-metric-instruments-interface

2e550f8

Merge branch 'main' into feat/add-update-metric-instruments-interface

7c67a60

vmarchaud approved these changes Nov 20, 2022

View reviewed changes

vmarchaud merged commit b29deee into open-telemetry:main Nov 20, 2022

legendecas mentioned this pull request Nov 21, 2022

chore(instrumentation): pin @opentelemetry/sdk-metrics to v1.8.0 #3431

Merged

2 tasks

legendecas mentioned this pull request Feb 22, 2023

[api] Implement a delegating no-op MeterProvider #3622

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(instrumentation): add new setMeterInstruments method #3267

feat(instrumentation): add new setMeterInstruments method #3267

osherv commented Sep 19, 2022

codecov bot commented Sep 19, 2022 •

edited

Loading

osherv commented Sep 20, 2022

legendecas left a comment

osherv commented Nov 6, 2022

legendecas left a comment

legendecas left a comment

dyladan Nov 8, 2022

osherv Nov 10, 2022

legendecas left a comment

legendecas Nov 11, 2022

legendecas Nov 11, 2022

osherv Nov 13, 2022

vmarchaud Nov 13, 2022

osherv Nov 14, 2022

legendecas Nov 15, 2022

legendecas Nov 18, 2022

legendecas commented Nov 15, 2022

	"@opentelemetry/sdk-metrics": "^1.8.0",
	"@opentelemetry/sdk-metrics": "1.8.0",

feat(instrumentation): add new setMeterInstruments method #3267

feat(instrumentation): add new setMeterInstruments method #3267

Conversation

osherv commented Sep 19, 2022

codecov bot commented Sep 19, 2022 • edited Loading

Codecov Report

osherv commented Sep 20, 2022

legendecas left a comment

Choose a reason for hiding this comment

osherv commented Nov 6, 2022

legendecas left a comment

Choose a reason for hiding this comment

legendecas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legendecas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legendecas commented Nov 15, 2022

codecov bot commented Sep 19, 2022 •

edited

Loading