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

otlp-transformer requires BigInt #4350

Closed
maximelkin opened this issue Dec 8, 2023 · 2 comments · Fixed by #4484
Closed

otlp-transformer requires BigInt #4350

maximelkin opened this issue Dec 8, 2023 · 2 comments · Fixed by #4484
Labels
bug Something isn't working pkg:otlp-transformer priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization up-for-grabs Good for taking. Extra help will be provided by maintainers

Comments

@maximelkin
Copy link

maximelkin commented Dec 8, 2023

What happened?

Steps to Reproduce

install @opentelemetry/otlp-transformer
build bundle for browsers with opentelemetry tracing

Expected Result

@opentelemetry/otlp-transformer fallbacks if BigInt not exists

Actual Result

it doesn't
https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/otlp-transformer/src/common/index.ts#L21

Additional Details

As temporary workaround include bigint polyfill unconditionally for old browsers

OpenTelemetry Setup Code

No response

package.json

{
  "dependencies": {
    "@opentelemetry/api": "1.7.0",
    "@opentelemetry/core": "1.18.1",
    "@opentelemetry/exporter-trace-otlp-http": "0.45.1",
    "@opentelemetry/instrumentation": "0.45.1",
    "@opentelemetry/instrumentation-fetch": "0.45.1",
    "@opentelemetry/instrumentation-xml-http-request": "0.45.1",
    "@opentelemetry/resources": "1.18.1",
    "@opentelemetry/sdk-trace-base": "1.18.1",
    "@opentelemetry/sdk-trace-web": "1.18.1",
    "@opentelemetry/semantic-conventions": "1.18.1"
  }
}

Relevant log output

No response

@maximelkin maximelkin added bug Something isn't working triage labels Dec 8, 2023
@t2t2
Copy link
Contributor

t2t2 commented Dec 13, 2023

This kinda depends on decisions to be done in #4168 - likely anyting as old as IE would only be supported as not by default/user must add polyfills (be it with babel/preset-env w/ polyfills based on usage or whatever)

Other browsers have good enough support for BigInt https://caniuse.com/bigint

@jordanbd
Copy link

The use of BigInt within opentelemetry-js is a relatively recent change and localised entirely to the changes introduced from this PR. The PR even went to some effort to try and offer fallbacks when BigInt is unavailable (see here) but missed the NANOSECONDS constant.

I have a use case to support Chrome 40+ (but unfortunately below 67 where BigInt support is introduced). Given the near impossibility of polyfilling BigInt it would be nice if the NANOSECONDS constant could be guarded somehow, because even if #4168 results in me being stuck on an older version of opentelemetry-js I at least get the benefits of PR 4220.

@pichlermarc pichlermarc added priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization pkg:otlp-transformer up-for-grabs Good for taking. Extra help will be provided by maintainers and removed triage labels Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:otlp-transformer priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization up-for-grabs Good for taking. Extra help will be provided by maintainers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants