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.

Sign up for GitHub

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

fix: refactoring and solves #2321 #2380

Closed
wants to merge 2 commits into from
Closed

fix: refactoring and solves #2321 #2380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e68438
fix: refacrtoring and solves #2321
niko-achilles Jul 27, 2021
73c873b
Merge branch 'main' into useBeacon
niko-achilles Jul 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions ...ges/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ export abstract class CollectorExporterBrowserBase<
ExportItem,
ServiceRequest
> {
private DEFAULT_HEADERS = {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


protected _headers: Record<string, string>;
private _useXHR: boolean = false;

Expand All @@ -43,16 +49,20 @@ export abstract class CollectorExporterBrowserBase<
super(config);
this._useXHR =
!!config.headers || typeof navigator.sendBeacon !== 'function';

if (this._useXHR) {
this._headers = Object.assign(
{},
parseHeaders(config.headers),
baggageUtils.parseKeyPairsIntoRecord(
this._headers = {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@niko-achilles niko-achilles Aug 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@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 ?

...this.DEFAULT_HEADERS,
...parseHeaders(config.headers),
...baggageUtils.parseKeyPairsIntoRecord(
getEnv().OTEL_EXPORTER_OTLP_HEADERS
)
);
),
};
} else {
this._headers = {};
this._headers = {
...this.DEFAULT_BLOB_TYPE,
...parseHeaders({ type: config.blobType }),
};
}
}

Expand Down Expand Up @@ -94,7 +104,7 @@ export abstract class CollectorExporterBrowserBase<
if (this._useXHR) {
sendWithXhr(body, this.url, this._headers, _onSuccess, _onError);
} else {
sendWithBeacon(body, this.url, _onSuccess, _onError);
sendWithBeacon(body, this.url, this._headers, _onSuccess, _onError);
}
});
this._sendingPromises.push(promise);
Expand Down
10 changes: 3 additions & 7 deletions packages/opentelemetry-exporter-collector/src/platform/browser/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,12 @@ import * as collectorTypes from '../../types';
export function sendWithBeacon(
body: string,
url: string,
headers: Record<string, string>,
onSuccess: () => void,
onError: (error: collectorTypes.CollectorExporterError) => void
) {
if (navigator.sendBeacon(url, body)) {
const blob = new Blob([body], headers);
if (navigator.sendBeacon(url, blob)) {
diag.debug('sendBeacon - can send', body);
onSuccess();
} else {
Expand Down Expand Up @@ -56,13 +58,7 @@ export function sendWithXhr(
const xhr = new XMLHttpRequest();
xhr.open('POST', url);

const defaultHeaders = {
'Accept': 'application/json',
'Content-Type': 'application/json',
};

Object.entries({
...defaultHeaders,
...headers,
}).forEach(([k, v]) => {
xhr.setRequestHeader(k, v);
Expand Down
4 changes: 4 additions & 0 deletions packages/opentelemetry-exporter-collector/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,10 @@ export interface ExportServiceError {
* Collector Exporter base config
*/
export interface CollectorExporterConfigBase {
/**
* type of the data contained in the Blob
*/
blobType?: string;
headers?: Partial<Record<string, unknown>>;
hostname?: string;
attributes?: SpanAttributes;
Expand Down
25 changes: 21 additions & 4 deletions packages/opentelemetry-exporter-collector/test/browser/CollectorMetricExporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ describe('CollectorMetricExporter - web', () => {
},
'double-observer2'
);
const recorder: Metric<BoundValueRecorder> &
ValueRecorder = mockValueRecorder();
const recorder: Metric<BoundValueRecorder> & ValueRecorder =
mockValueRecorder();
counter.add(1);
recorder.record(7);
recorder.record(14);
Expand Down Expand Up @@ -93,10 +93,11 @@ describe('CollectorMetricExporter - web', () => {
it('should successfully send metrics using sendBeacon', done => {
collectorExporter.export(metrics, () => {});

setTimeout(() => {
setTimeout(async () => {
const args = stubBeacon.args[0];
const url = args[0];
const body = args[1];
const blob: Blob = args[1];
const body = await blob.text();
const json = JSON.parse(
body
) as collectorTypes.opentelemetryProto.collector.metrics.v1.ExportMetricsServiceRequest;
Expand Down Expand Up @@ -158,6 +159,22 @@ describe('CollectorMetricExporter - web', () => {
});
});

it('should successfully send blob type using sendBeacon', done => {
collectorExporter.export(metrics, () => {});
const expectedType = 'application/json';

setTimeout(() => {
const args = stubBeacon.args[0];
const blob: Blob = args[1];
const { type } = blob;

assert.strictEqual(type, expectedType);
assert.strictEqual(stubBeacon.callCount, 1);
assert.strictEqual(stubOpen.callCount, 0);
done();
});
});

it('should log the successful message', done => {
// Need to stub/spy on the underlying logger as the "diag" instance is global
const spyLoggerDebug = sinon.stub(diag, 'debug');
Expand Down
Loading