fix: refactoring and solves #2321 #2380

niko-achilles · 2021-07-27T12:57:47Z

Which problem is this PR solving?

fixes Traces send by http protocol fail in new Otel collector receiver #2321
refactored commit fix: headers are appended to existing one (open-telemetry#2335) #2357
refactoring of CollectorExporterBrowserBase, utils and test cases for CollectorTraceExporter
extended test cases for CollectorTraceExporter

Short description of the changes

blob type as header is send with beacon
user has the ability to set the blobType in collectorConfiguration
after looking at the codebase , i think that the utils.ts should not have logic behavior for correcting the headers, given in the code base , the CollectorExporterBrowserBase knows about headers and logic

codecov · 2021-07-27T13:03:34Z

Codecov Report

Merging #2380 (9afe781) into main (e089984) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 9afe781 differs from pull request most recent head 73c873b. Consider uploading reports for the commit 73c873b to get more accurate results

@@            Coverage Diff             @@
##             main    #2380      +/-   ##
==========================================
- Coverage   92.36%   92.34%   -0.03%     
==========================================
  Files         128      128              
  Lines        4244     4244              
  Branches      867      867              
==========================================
- Hits         3920     3919       -1     
- Misses        324      325       +1

Impacted Files	Coverage Δ
...emetry-core/src/platform/node/RandomIdGenerator.ts	`87.50% <0.00%> (-6.25%)`	⬇️

niko-achilles · 2021-07-29T15:07:43Z

saved as draft until a result merge output from #2336

in short this commit has tests that pass

CollectorTraceExporter - web

when "sendBeacon" is available

and user defines headers
✓ should successfully send user defined headers using XMLHttpRequest
✓ Request Headers should contain "Content-Type" header
✓ Request Headers should contain "Accept" header
and user does not define headers
✓ should successfully send the spans using sendBeacon
✓ should successfully send expected blob type using sendBeacon
and user sets headers as undefined
✓ should successfully send the spans using sendBeacon
✓ should successfully send expected blob type using sendBeacon
and user defines blob type
✓ should successfully send the spans using sendBeacon
✓ should successfully send user defined blob type using sendBeacon
and user does not define blob type
✓ should successfully send the spans using sendBeacon
✓ should successfully send expected blob type using sendBeacon
and user defines blob type as undefined
✓ should successfully send the spans using sendBeacon
✓ should successfully send expected blob type using sendBeacon
should be able to handle logs
✓ when successful with can-send message
✓ when errored with cannot send message

when "sendBeacon" is NOT available

and user defines headers
✓ Request Headers should contain user defined headers
✓ Request Headers should contain "Content-Type" header
✓ Request Headers should contain "Accept" header
and user does not define headers
✓ should successfully send the spans using XMLHttpRequest
✓ Request Headers should contain "Content-Type" header
✓ Request Headers should contain "Accept" header
and user sets headers as undefined
✓ should successfully send the spans using XMLHttpRequest
✓ Request Headers should contain "Content-Type" header
✓ Request Headers should contain "Accept" header
and user defines blob type
✓ should successfully send the spans using XMLHttpRequest
✓ Request Headers should contain "Content-Type" header
✓ Request Headers should contain "Accept" header
and user does not define blob type
✓ should successfully send the spans using XMLHttpRequest
✓ Request Headers should contain "Content-Type" header
✓ Request Headers should contain "Accept" header
and user defines blob type as undefined
✓ should successfully send the spans using XMLHttpRequest
✓ Request Headers should contain "Content-Type" header
✓ Request Headers should contain "Accept" header
should be able to handle logs
✓ should log the successful message
✓ should log the error message

CollectorMetricExporter - web

when "sendBeacon" is available

✓ should successfully send metrics using sendBeacon
✓ should successfully send blob type using sendBeacon
... tests cases can be extended with further tests in context of review ....

dyladan · 2021-08-02T15:02:50Z

Before you take this PR out of draft status please give it a meaningful name. The PR name goes into the git log as the main message when the PR is squashed and having to refer to an external issue to learn its meaning is a frustrating workflow.

obecny · 2021-08-02T15:47:29Z

packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts

@@ -33,6 +33,12 @@ export abstract class CollectorExporterBrowserBase<
  ExportItem,
  ServiceRequest
 > {
+  private DEFAULT_HEADERS = {


why do you think it's better to keep defaults on instance instead in utils ?

correct me if i am wrong.

Who owns the config ?

The CollectorExporterBrowserBase

Owning the data means working with the data and correcting the data if needed, like applying default headers or type for blob property bag.

What are the methods sendWithXhr, sendWithBeacon under
opentelemetry-js/packages/opentelemetry-exporter-collector/src/platform/browser/util.ts ?

Are roles with the responsibility to send data with a protocol.

Protocol is the name of each function and data is the arguments that each function define.

obecny · 2021-08-02T15:50:22Z

packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts

+    Accept: 'application/json',
+    'Content-Type': 'application/json',
+  };
+  private DEFAULT_BLOB_TYPE = { type: 'application/json' };


imho it should not be a part of CollectorExporterBrowserBase.ts as it is only used if beacon is used -> move to util

the logic deciding if beacon is used , is a decision that CollectorExporterBase already has made before sendWithBeacon util function.

obecny · 2021-08-02T15:53:44Z

packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts

-        {},
-        parseHeaders(config.headers),
-        baggageUtils.parseKeyPairsIntoRecord(
+      this._headers = {


I would prefer to have 2 util functions

prepareXHRHeaders / getXHRHeaders / parseXHRHeaders etc.

prepareBeaconHeaders ...

Currently the logic will be scattered between util and exporter, where exporter should not contain anymore logic then it is necessary and those functions will be straightforward and easy to understand. If someone want to change it you will change util class not the exporter.

actually i think that the correct place would be to put the utility functions like the ones you mention here under
https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-exporter-collector/src/util.ts

What do you think ?
My motivation is because in this file there is already a helper method parseheaders.
https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-exporter-collector/src/util.ts#L23

Or a better one, what do you think ?

rename the file util to send under https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-exporter-collector/src/platform/browser/util.ts
and export from it the functions sendWithXhr, sendWithBeacon.

Then introduce a util.ts under opentelemetry-js/blob/main/packages/opentelemetry-exporter-collector/src/platform/browser/
with the functions as helpers you mentioned above

prepareXHRHeaders / getXHRHeaders / parseXHRHeaders etc.

prepareBeaconHeaders ...

Then send module and util module can be imported to CollectorExporterBase and collector exporter base can work with them.

@obecny a more consistent solution is given by @MSNev in context of blob and sendWithBeacon here:
#2336 (comment)

So can i move the default header for xhr inside the send method of CollectorExporterBase ?
i mean this logic under util.ts
https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-exporter-collector/src/platform/browser/util.ts#L59

to send method here:
https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts#L94

it should then look something like this:

if(this.xhr){ const headers = {...{"Content-Type":"application/json", "Accept": "application/json"},...this.headers} sendWithXhr(body, this.url, headers, _onSuccess, _onError); }

i would then close this PR and open a new one in order to be in context of xhr only , given the blob context PR will be handled here: #2336

@obecny , @MSNev, what do you think for the above refactoring proposal for xhr ?

niko-achilles · 2021-08-10T12:22:41Z

closing this PR because of #2336

fix: refacrtoring and solves open-telemetry#2321

6e68438

niko-achilles requested review from dyladan, Flarna, johnbley, legendecas, markwolff, mayurkale22, MSNev, mwear, naseemkullah, obecny, OlivierAlbertini, rauno56 and vmarchaud as code owners July 27, 2021 12:57

niko-achilles mentioned this pull request Jul 27, 2021

feat: use Blob in sendBeacon to add application/json type #2336

Merged

Merge branch 'main' into useBeacon

73c873b

niko-achilles marked this pull request as draft July 27, 2021 23:00

obecny reviewed Aug 2, 2021

View reviewed changes

Flarna changed the title ~~fix: refacrtoring and solves #2321~~ fix: refactoring and solves #2321 Aug 4, 2021

niko-achilles closed this Aug 10, 2021

niko-achilles deleted the useBeacon branch August 10, 2021 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: refactoring and solves #2321 #2380

fix: refactoring and solves #2321 #2380

niko-achilles commented Jul 27, 2021 •

edited

codecov bot commented Jul 27, 2021 •

edited

niko-achilles commented Jul 29, 2021 •

edited

dyladan commented Aug 2, 2021

obecny Aug 2, 2021

niko-achilles Aug 2, 2021

obecny Aug 2, 2021

niko-achilles Aug 2, 2021

obecny Aug 2, 2021

niko-achilles Aug 2, 2021

niko-achilles Aug 2, 2021

niko-achilles Aug 4, 2021 •

edited

niko-achilles commented Aug 10, 2021

fix: refactoring and solves #2321 #2380

fix: refactoring and solves #2321 #2380

Conversation

niko-achilles commented Jul 27, 2021 • edited

Which problem is this PR solving?

Short description of the changes

codecov bot commented Jul 27, 2021 • edited

Codecov Report

niko-achilles commented Jul 29, 2021 • edited

dyladan commented Aug 2, 2021

obecny Aug 2, 2021

Choose a reason for hiding this comment

niko-achilles Aug 2, 2021

Choose a reason for hiding this comment

obecny Aug 2, 2021

Choose a reason for hiding this comment

niko-achilles Aug 2, 2021

Choose a reason for hiding this comment

obecny Aug 2, 2021

Choose a reason for hiding this comment

niko-achilles Aug 2, 2021

Choose a reason for hiding this comment

niko-achilles Aug 2, 2021

Choose a reason for hiding this comment

niko-achilles Aug 4, 2021 • edited

Choose a reason for hiding this comment

niko-achilles commented Aug 10, 2021

niko-achilles commented Jul 27, 2021 •

edited

codecov bot commented Jul 27, 2021 •

edited

niko-achilles commented Jul 29, 2021 •

edited

niko-achilles Aug 4, 2021 •

edited