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

refactor(opentelemetry-instrumentation-http): make _getConfig public and rename to getConfig #2201

Closed
wants to merge 1 commit into from
Closed

refactor(opentelemetry-instrumentation-http): make _getConfig public and rename to getConfig #2201

wants to merge 1 commit into from

Conversation

mottibec
Copy link
Contributor

Which problem is this PR solving?

  • we are creating socket.io instrumentation at Aspecto.
    The problem occurs when socket.io uses polling as the transport method and sends an HTTP POST request, this causes a trace to be generated by the HTTP instrumentation.
    The workaround we thought about was to pass the instance of HttpInstrumentation to our socket.io instrumentation and modify the HttpInstrumentationConfig.ignoreIncomingPaths to include the path of the socket.io endpoint.
    In order to do that, we need to publicize the _getConfig method so we can merge the new config with the existing one and call setConfig.

Short description of the changes

  • rename _getConfig to getConfig
  • change access modifier from private to public

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 12, 2021

CLA Signed

The committers are authorized under a signed CLA.

@obecny
Copy link
Member

obecny commented May 12, 2021

If I remember correctly the idea was to update main instrumentation package and make this method public on the Instrumentation level, instead of each individual instrumentation. I could swear we had a ticket for that.

@obecny
Copy link
Member

obecny commented May 12, 2021

here it is -> #2174

@codecov
Copy link

codecov bot commented May 12, 2021

Codecov Report

Merging #2201 (c624e8d) into main (5dcec45) will increase coverage by 0.48%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2201      +/-   ##
==========================================
+ Coverage   92.27%   92.75%   +0.48%     
==========================================
  Files         122      141      +19     
  Lines        4066     5055     +989     
  Branches      833     1042     +209     
==========================================
+ Hits         3752     4689     +937     
- Misses        314      366      +52     
Impacted Files Coverage Δ
...ges/opentelemetry-instrumentation-http/src/http.ts 95.61% <100.00%> (+0.79%) ⬆️
...e/src/baggage/propagation/HttpBaggagePropagator.ts 96.87% <0.00%> (-1.38%) ⬇️
...s/opentelemetry-tracing/src/BasicTracerProvider.ts 96.10% <0.00%> (-0.51%) ⬇️
...ckages/opentelemetry-core/src/utils/environment.ts 95.83% <0.00%> (ø)
...ges/opentelemetry-tracing/src/NoopSpanProcessor.ts
...ry-exporter-collector/src/CollectorExporterBase.ts 92.30% <0.00%> (ø)
...kages/opentelemetry-exporter-collector/src/util.ts 100.00% <0.00%> (ø)
...kages/opentelemetry-web/src/StackContextManager.ts 97.05% <0.00%> (ø)
...es/opentelemetry-context-zone-peer-dep/src/util.ts 100.00% <0.00%> (ø)
.../opentelemetry-exporter-collector/src/transform.ts 88.69% <0.00%> (ø)
... and 19 more

@rauno56
Copy link
Member

rauno56 commented May 21, 2021

@mottibec, would you like to take a stab at adding it to the base instrumentation class?

@mottibec
Copy link
Contributor Author

mottibec commented May 23, 2021

@mottibec, would you like to take a stab at adding it to the base instrumentation class?

@rauno56 Sure, this looks like an excellent issue to start contributing to Otel.
done in #2224

@mottibec mottibec closed this May 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants