Skip to content

feat: browser support for exporter-trace-otlp-proto #3208

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

Merged
legendecas merged 51 commits into from
Jan 20, 2023
Merged

feat: browser support for exporter-trace-otlp-proto #3208

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
915ce52
add node & browser platform for exporter-trace-otlp-proto
pkanal Aug 24, 2022
87327f0
add browser support for proto base exporter
pkanal Aug 25, 2022
b7c8c19
add base browser class with xhr support
pkanal Aug 25, 2022
1fec3b3
add browser class for proto trace exporter
pkanal Aug 25, 2022
4214129
slight tweaks to make it work
pkanal Aug 25, 2022
d33f9bb
send data as blob to avoid making sync xhr requests
pkanal Aug 26, 2022
3dd4e17
fix lint
pkanal Oct 3, 2022
35c193f
remove console.logs and add browser proto example
pkanal Oct 3, 2022
5ed212d
cleanup and start adding tests
pkanal Oct 12, 2022
933d37e
Merge branch 'main' of github.com:pkanal/opentelemetry-js into proto-…
scheler Nov 7, 2022
9953561
Undo formatting changes
scheler Nov 7, 2022
5e87c85
exporter-trace-otlp-proto: fix compile errors
scheler Nov 8, 2022
1616d2b
Misc updates from review comments
scheler Nov 9, 2022
e3043b0
Merge branch 'main' of github.com:pkanal/opentelemetry-js into proto-…
scheler Nov 10, 2022
0020c98
Adding changelog entry
scheler Nov 10, 2022
6125ec2
Merge branch 'open-telemetry:main' into proto-exporter-browser
scheler Nov 10, 2022
c420fd0
Reverting format changes not needed
scheler Nov 12, 2022
f260db8
Merge branch 'main' into proto-exporter-browser
legendecas Nov 15, 2022
fb6cfd2
Merge branch 'open-telemetry:main' into proto-exporter-browser
scheler Nov 16, 2022
3d9ae04
Moving the send function into the class for browser case.
scheler Nov 29, 2022
46b665a
Merge branch 'main' into proto-exporter-browser
scheler Nov 30, 2022
2000c41
Adjust indentation to fix lint errors
scheler Nov 30, 2022
6366e4b
Remove template parameter that's not needed
scheler Nov 30, 2022
50e1af0
Apply review changes
scheler Dec 4, 2022
302e09c
Merge branch 'main' into proto-exporter-browser
scheler Dec 4, 2022
a16fcf5
fix the import path
scheler Dec 5, 2022
266d293
Addressing lint errors
scheler Dec 5, 2022
4cd39c0
Merge branch 'main' into proto-exporter-browser
scheler Dec 6, 2022
ed1835e
Merge branch 'open-telemetry:main' into proto-exporter-browser
scheler Dec 10, 2022
84be752
Explicit imports for browser case
scheler Dec 12, 2022
d92f5fe
More explicit exports
scheler Dec 13, 2022
61c7be7
Add missing exports
scheler Dec 13, 2022
3eed4bb
Address lint issues with export statements
scheler Dec 14, 2022
c618265
Adding missing exports
scheler Dec 14, 2022
cbabbf1
Adding missing export
scheler Dec 14, 2022
519fa7c
Using import from top level folder
scheler Dec 14, 2022
5f67e56
Merge branch 'open-telemetry:main' into proto-exporter-browser
scheler Dec 28, 2022
ad2aa0e
Trigger Build
scheler Dec 28, 2022
e57f0f2
Merge branch 'proto-exporter-browser' of github.com:pkanal/openteleme…
scheler Dec 28, 2022
b26fd7f
Merge branch 'main' into proto-exporter-browser
pichlermarc Jan 2, 2023
f421989
Update experimental/packages/exporter-trace-otlp-proto/test/browser/C…
scheler Jan 2, 2023
8064d5b
Remove trailing comma
scheler Jan 2, 2023
2678ff3
Remove blank line to fix lint error
scheler Jan 2, 2023
c898ea2
Fixes based on testing opentelemetry-web/fetch-proto
scheler Jan 6, 2023
d48503a
Add additional missing export
scheler Jan 12, 2023
a800550
Skip hex conversion of traceId for the protobuf
scheler Jan 12, 2023
598bc56
Add esm/esnext builds for the proto packages
scheler Jan 12, 2023
5596d13
Merge branch 'main' into proto-exporter-browser
scheler Jan 13, 2023
1788269
Merge branch 'main' into proto-exporter-browser
legendecas Jan 17, 2023
2e0c4ec
Merge branch 'main' into proto-exporter-browser
scheler Jan 19, 2023
9429864
Trigger Build
scheler Jan 19, 2023
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
Prev Previous commit
Next Next commit
exporter-trace-otlp-proto: fix compile errors
  • Loading branch information
@scheler
scheler committed Nov 8, 2022
commit 5e87c85179893cebaec6a3a97732b14ca0f9758f
2 changes: 1 addition & 1 deletion experimental/packages/exporter-trace-otlp-proto/src/platform/browser/OTLPTraceExporter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export class OTLPTraceExporter
return typeof config.url === 'string'
Copy link
Contributor

@MSNev MSNev Dec 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggestion
This is a little more readable, performant (and minifiable) by rewriting as

let endpoint = config.url;
if (!endpoint || endpoint !== 'string') {
    let env = getEnv();
    let tracesEndpoint = env.OTEL_EXPORTER_OTLP_TRACES_ENDPOINT;
    if (tracesEndpoint) {
        endpoint = appendRootPathToUrlIfNeeded(tracesEndpoint);
    } else {
        let otlpEndpoint = env.OTEL_EXPORTER_OTLP_ENDPOINT;
        if (otlpEndpoint) {
            endpoint = appendResourcePathToUrl(otlpEndpoint, DEFAULT_COLLECTOR_RESOURCE_PATH);
        }
    }
}

return endpoint || DEFAULT_COLLECTOR_URL;

As well as less error prone for someone coming looking at this after you

Copy link
Contributor

@scheler scheler Dec 28, 2022

Choose a reason for hiding this comment

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

@MSNev The function getDefaultUrl is written this way in multiple packages. I suggest we take this up in a separate PR since it's unrelated to this PR.

Copy link
Contributor

@MSNev MSNev Jan 9, 2023

Choose a reason for hiding this comment

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

Can we raise an issue for this then, otherwise it will not get done. This can include the suggestion I have above as the implementation.

? config.url
: getEnv().OTEL_EXPORTER_OTLP_TRACES_ENDPOINT.length > 0
? appendRootPathToUrlIfNeeded(getEnv().OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, DEFAULT_COLLECTOR_RESOURCE_PATH)
? appendRootPathToUrlIfNeeded(getEnv().OTEL_EXPORTER_OTLP_TRACES_ENDPOINT)
: getEnv().OTEL_EXPORTER_OTLP_ENDPOINT.length > 0
? appendResourcePathToUrl(getEnv().OTEL_EXPORTER_OTLP_ENDPOINT, DEFAULT_COLLECTOR_RESOURCE_PATH)
: DEFAULT_COLLECTOR_URL;
Expand Down
2 changes: 1 addition & 1 deletion experimental/packages/exporter-trace-otlp-proto/test/browser/CollectorTraceExporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ describe('OTLPTraceExporter - web', () => {
hostname: 'foo',
url: 'http://foo.bar.com',
};
collectorTraceExporter = new OTLPTraceExporter(collectorConfig)
collectorTraceExporter = new OTLPTraceExporter(collectorExporterConfig)
});
});

Expand Down