Skip to content
This repository was archived by the owner on Oct 3, 2023. It is now read-only.

Fix propagation breaking for https.get#657

Merged
mayurkale22 merged 1 commit into
census-instrumentation:masterfrom
loneparadox:fix-https-get-wrap-propagation
Oct 3, 2019
Merged

Fix propagation breaking for https.get#657
mayurkale22 merged 1 commit into
census-instrumentation:masterfrom
loneparadox:fix-https-get-wrap-propagation

Conversation

@loneparadox

Copy link
Copy Markdown
Contributor

shameful copy from the http plugin code, to fix the
https.get same as http.get.

#653

Copied from https://github.com/census-instrumentation/opencensus-node/pull/324/files

shamelful copy from the http plugin code, to fix the
`https.get` same as `http.get`.
@googlebot

Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #657 into master will increase coverage by 0.16%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #657      +/-   ##
==========================================
+ Coverage   95.43%   95.59%   +0.16%     
==========================================
  Files         148       10     -138     
  Lines       10730      659   -10071     
  Branches      799       52     -747     
==========================================
- Hits        10240      630    -9610     
+ Misses        490       29     -461
Impacted Files Coverage Δ
test/test-jaeger-format.ts 96.22% <0%> (-0.39%) ⬇️
src/index.ts 100% <0%> (ø) ⬆️
src/validators.ts 100% <0%> (ø) ⬆️
test/test-prometheus-stats.ts 100% <0%> (ø) ⬆️
src/internal/string-utils.ts
src/stats/metric-producer.ts
src/http-stats.ts
src/common/noop-logger.ts
src/instana.ts
scripts/utils.ts
... and 134 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67aca05...d673041. Read the comment docs.

@loneparadox

Copy link
Copy Markdown
Contributor Author

@googlebot I signed it!

@googlebot

Copy link
Copy Markdown

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Oct 3, 2019
'get',
this.getPatchHttpsOutgoingRequest()
);
shimmer.wrap(this.moduleExports, 'get', () => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mayurkale22 I could not figure out how to change the http module to put this code into a method there, and then I could have just referenced it from this https module...

But everything references a specific version(test and code) so I did not spend more time in trying to figure out how to do this.
Hence the reason I ended up coping the same code from the http module and just dumping it here.

@draffensperger draffensperger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @loneparadox for this contribution! I'm OK with the duplication personally particularly given that we are transitioning efforts to OpenTelemetry JS.

@mayurkale22 WDYT?

@mayurkale22

Copy link
Copy Markdown
Member

I'm OK with the duplication personally particularly given that we are transitioning efforts to OpenTelemetry JS.
@mayurkale22 WDYT?

I agree with @draffensperger. I will fine with merging like this.

@mayurkale22

Copy link
Copy Markdown
Member

I'd like to encourage you to consider looking the https://github.com/open-telemetry/opentelemetry-js repo. This would be the next major release of OpenCensus + OpenTracing merger. We are getting ready for Alpha release, within 1-2 days.

@mayurkale22 mayurkale22 merged commit 80db31f into census-instrumentation:master Oct 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants