fix: Handle missing config in middleware when OTEL_SDK_DISABLED=true#2231
fix: Handle missing config in middleware when OTEL_SDK_DISABLED=true#2231hannahramadan wants to merge 4 commits intoopen-telemetry:mainfrom
Conversation
hannahramadan
requested review from
a team,
ahayworth,
arielvalentin,
dazuma,
ericmustin,
fbogsany,
kaylareopelle,
mwear,
plantfansam,
robbkidd,
robertlaurin,
simi and
xuan-cao-swi
as code owners
hannahramadan
changed the title
|
@hannahramadan is this not something that should be handled in Base? Also, any chance that you could add some test coverage for this? |
Base could make sense because it looks like this same issue could occur in a few other instrumentations. So we'd either need the |
|
|
||
| def call(type:, requests: nil, call: nil, method: nil, metadata: nil) | ||
| return yield if instrumentation_config.empty? | ||
| return yield unless Grpc::Instrumentation.instance.installed? |
There was a problem hiding this comment.
This check to see if instrumentation was installed, along with the checks in gruf/interceptors/client and gruf/interceptors/server aren't technically equivalent to the previous behavior of checking if the configs were empty.
Is installed? the right check here, or should we handle this differently? I'm unsure about the original intent of using an config empty vs installed to guard.
There was a problem hiding this comment.
@AS-AlStar would you mind sharing more information about your intent here?
| @config = {} | ||
| @installed = false | ||
| @options = options | ||
| @config = config_options({}) |
There was a problem hiding this comment.
As this is a change to base, I would love some extra eyes here. I noted one downstream impact of checking for empty configs not really being able to exist anymore, since now they wont ever really be.
thompson-tomo
dismissed
arielvalentin’s stale review
Original solution which was approved has been reverted and different solution implemented affecting different set of gems.
@arielvalentin ah because if we have |
2 participants
When
OTEL_SDK_DISABLED=true, the SDK skips configuration and Rack instrumentation's config is an empty hash. When attempting to access nil values, aNoMethodErrorwas raised. This same issue seemed to exist in at least excon, grpc, and gruf.We now initialize config with defaults in the base class ensure config always has valid default values, whether or not install() was called.
This is slightly redundant since @config gets overwritten in the
installmethod, the first is a "just in case" install() is never called.closes #2230