fix(detector-gpc): suppress tracing when requesting for GCP metadata

[david-luna]

Which problem is this PR solving?

This is a 1st PR for fixing #2320. Some of the detectors are using http or other instrumented modules to get resource related data. In this case GCP resource detector is using http module to get info and this results on some spans being exported. These spans are not related to the instrumented service so it shouldn't be there

Short description of the changes

• Update GcpDetector class to implement DetectorSync interface
• use context.with along with suppresTracing API to make sure spans are not exported
• add a test for it

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.43%. Comparing base (dfb2dff) to head (0aaaf29).
Report is 188 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2321      +/-   ##
==========================================
- Coverage   90.97%   90.43%   -0.55%     
==========================================
  Files         146      148       +2     
  Lines        7492     7326     -166     
  Branches     1502     1518      +16     
==========================================
- Hits         6816     6625     -191     
- Misses        676      701      +25     

Files	Coverage Δ
...resource-detector-gcp/src/detectors/GcpDetector.ts	96.22% <100.00%> (+0.22%)	⬆️

... and 62 files with indirect coverage changes

[trentm]

nit: missing license header, and mixed 4- and 2-space indents in this file. Unfortunately the eslint setup doesn't lint .js files.

[david-luna]

thanks! maybe we could also make eslint work in js (but not in this PR)

[trentm]

Definite not this PR.

[trentm]

Definite not this PR.

[trentm]

(I guess I forgot to hit "save" in my first review.)

Wow this was a fair amount of effort to write a test case for this.
I don't know how others would feel, but I wouldn't require similar test code be added for suppressTracing usage in other detectors.

[david-luna]

Yup it was a fair amount of work 😓. But I needed

• the sdk running so I had to create the fixture
• to avoid nock since real requests won't happen so we won't be really testing
• gcp-metadata to get responses. I could let it fail but I found that the request time vary in a way that makes the test flaky.

there is another approach I can take but 1st I wanted some feeedback for this one. If we finally use this one I may be inclined to move the mock server to @opentelemetry/contrib-test-utils

[david-luna]

About the 3rd point in my previous comment I found that if the async attributes of the resource are resolved after calling sdk.shutdown() we loose some spans. Without knowing to much about the mechanics of the processors/exporters I'd guess that these spans are not yet in the queue that is flushed during shutdown. I think this could be a potential issue for apps that have a short execution time (thinking on lamba or similar)

[david-luna]

@trentm the other approach seems to work and it's shorter 🎉 (although we're getting a resource with empty values). Changes are

• remove the mock server
• trigger the detection again and waitForAsyncAttributes

diff --git a/detectors/node/opentelemetry-resource-detector-gcp/test/fixtures/use-gcp-detector.js b/detectors/node/opentelemetry-resource-detector-gcp/test/fixtures/use-gcp-detector.js
index 0c71a081..f88d2ae4 100644
--- a/detectors/node/opentelemetry-resource-detector-gcp/test/fixtures/use-gcp-detector.js
+++ b/detectors/node/opentelemetry-resource-detector-gcp/test/fixtures/use-gcp-detector.js
@@ -78,6 +78,9 @@ server.listen(0, '127.0.0.1', async function () {
   });

   // flush any left spans
+  // NOTE: this adds extra requests but its necessary to make sure
+  // spans have the resouce and are queued in the exporter
+  await gcpDetector.detect().waitForAsyncAttributes();
   await sdk.shutdown();
   await new Promise(resolve => server.close(resolve));
 });

A couple of observations:

• test becomes slower since the requests are made again at the end of the fixture code
• this detector just falls back to empty string if the request fails but I'm not sure if this will work on other detectors

I'm keen to use this simplified way but it is possible we have to do the server mock for other detectors (I hope not). WDYT?

[david-luna]

After a discussion with @trentm I've decided to do a different approach:

• update detectors to the new DetectrSync interface. a PR per detector so they are small and easy to review.
• once merged add suppressTracing to the ones that need it