feat: gradually add metrics capabilities to Instrumentation::Base#1324
feat: gradually add metrics capabilities to Instrumentation::Base#1324zvkemp wants to merge 1 commit intoopen-telemetry:mainfrom
Conversation
zvkemp
force-pushed
the
base-metrics
branch
2 times, most recently
from
7def994 to
f060a57
Compare
zvkemp
force-pushed
the
base-metrics
branch
3 times, most recently
from
bccd9f5 to
5f393f8
Compare
zvkemp
force-pushed
the
base-metrics
branch
3 times, most recently
from
ef747e7 to
8a538f8
Compare
zvkemp
changed the title
zvkemp
requested review from
a team,
ahayworth,
arielvalentin,
dazuma,
ericmustin,
fbogsany,
kaylareopelle,
mwear,
plantfansam,
robbkidd,
robertlaurin,
simi and
xuan-cao-swi
as code owners
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
kaylareopelle
added
keep
robertlaurin
left a comment
robertlaurin
left a comment
There was a problem hiding this comment.
Lets get the metrics api merged into opentelemetry-api instead of working around it in this fashion.
| def instance | ||
| @instance ||= new(instrumentation_name, instrumentation_version, install_blk, | ||
| present_blk, compatible_blk, options) | ||
| @instance || SINGLETON_MUTEX.synchronize do |
There was a problem hiding this comment.
Can you explain the rational for why this mutex lock is necessary?
| module Instrumentation | ||
| # Extensions to Instrumentation::Base that handle metrics instruments. | ||
| # The goal here is to allow metrics to be added gradually to instrumentation libraries, | ||
| # without requiring that the metrics-sdk or metrics-api gems are present in the bundle |
There was a problem hiding this comment.
without requiring that the metrics-sdk or metrics-api gems are present in the bundle
Instrumentation base has a dependency on the opentelemetry-api gem, if we want to start adding metric support in the instrumentation libraries the way forward is to get the metrics api stable and merged in the api proper.
| !disabled_by_env_var? | ||
| end | ||
|
|
||
| def disabled_by_env_var? |
There was a problem hiding this comment.
I don't actually see where this is being used.
3 participants
Proposed implementation for adding optional metrics capabilities to existing instrumentation libraries.
See #1377, #1314, or zvkemp#1 (example of observables) for example implementations. (<- note that these PRs are based on top of this branch, so you may see some duplication between here and there)
Proposed api:
:metrics.TODO:
Kernel.requireand detect when Metrics is loaded (probably too perilous to really consider, as any instrumentations with conditional metrics will need to be re-evaluated at this time), or eagerly attempt to require the metrics-api gem when Instrumentation::Base is loaded.