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

Fix circular import #647

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

StoneDot
Copy link
Contributor

This PR fixes the circular import problems occurring when you load http2 or https module before http module.

For example, the following code reproduces a problem @opencensus-instrumentation-http module doesn't load properly.

const tracing = require('@opencensus/nodejs');
const zipkin = require('@opencensus/exporter-zipkin');
const assert = require('assert');

const tracer = tracing.start({samplingRate: 1.0}).tracer;

const https = require('https');
const http = require('http');

tracer.registerSpanEventListener(new zipkin.ZipkinTraceExporter({
    url: 'http://localhost:9411/api/v2/spans',
    serviceName: 'node.js-open-census-bug'
}));

http.get('http://example.com/', (res) => {
  res.setEncoding('utf8');
  let rawData = '';
  res.on('data', (chunk) => { rawData += chunk; });
  res.on('end', () => {
    console.log(rawData);
  });
});

The process of problem is as follows schematically:

  1. Load http2 module.
  2. require-in-the-middle load @opencensus-instrumentation-http2 module.
  3. @opencensus-instrumentation-http2 load @opencensus-instrumentation-http module.
  4. @opencensus-instrumentation-http load http.
  5. require-in-the-middle try to load @opencensus-instrumentation-http, but loading of @opencensus-instrumentation-http is already in progress. Therefore require statement return partial @opencensus-instrumentation-http.
  6. The problem happens.

@googlebot
Copy link

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.

@StoneDot
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@mayurkale22
Copy link
Member

@draffensperger and @hekike what do you think about this?

Copy link
Contributor

@draffensperger draffensperger left a comment

Choose a reason for hiding this comment

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

LGTM, though I think it's important to comment why this is needed.

@@ -23,6 +23,7 @@ import {
SpanKind,
TraceOptions,
} from '@opencensus/core';
import 'http';
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment for why this line is needed to avoid the circular import? And similar for the other case below?

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.

4 participants