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

chore: adding interceptor for getting headers before each request #2050

Merged
merged 5 commits into from
Apr 13, 2021

Conversation

obecny
Copy link
Member

@obecny obecny commented Mar 26, 2021

Which problem is this PR solving?

Short description of the changes

  • Adds optional interceptor for getting headers before each request

@codecov
Copy link

codecov bot commented Mar 26, 2021

Codecov Report

Merging #2050 (2f8e861) into main (4ef22ca) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 2f8e861 differs from pull request most recent head bb1ba37. Consider uploading reports for the commit bb1ba37 to get more accurate results

@@            Coverage Diff             @@
##             main    #2050      +/-   ##
==========================================
+ Coverage   92.73%   92.75%   +0.01%     
==========================================
  Files         137      138       +1     
  Lines        4915     4927      +12     
  Branches     1015     1017       +2     
==========================================
+ Hits         4558     4570      +12     
  Misses        357      357              
Impacted Files Coverage Δ
...elemetry-exporter-zipkin/src/platform/node/util.ts 96.66% <ø> (ø)
...ackages/opentelemetry-exporter-zipkin/src/types.ts 100.00% <ø> (ø)
...ackages/opentelemetry-exporter-zipkin/src/utils.ts 100.00% <100.00%> (ø)
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 100.00% <100.00%> (ø)

@obecny
Copy link
Member Author

obecny commented Apr 8, 2021

@open-telemetry/javascript-approvers ^^, thx

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Seems to be a lot of function indirection here for quite a simple feature. Why do we have _beforeSend and _getHeaders and prepareGetHeaders and getHeadersBeforeSend?

Seems like the easiest solution would be more like:

class ZipkinExporter {
  // ...
  _getExportHeaders?: () => Map<string, string>;

  constructor(config: zipkinTypes.ExporterConfig) {
    // ...
    if (typeof config._getExportHeaders === 'function') {
      this._getExportHeaders = config._getExportHeaders
    }
  }
}

and in send you just do headers = this.__getExportHeaders?.() ?? {};

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Approving. Feel free to change the name or keep it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for dynamic headers in Zipkin exporter
3 participants