open-telemetry / opentelemetry-js

OpenTelemetry JavaScript Client
https://opentelemetry.io
Apache License 2.0
2.75k stars 809 forks source link

[Browser] Define the support browser runtimes #4168

Closed MSNev closed 4 weeks ago

MSNev commented 1 year ago

As part of SDK 2.0 discussion we should define the minimum supported browser type runtime. ie. Not just the supported browsers but the language feature set for those browser runtimes

This would then include native or frameworks that "provide" a browser style environment and whether it supports the minumum would identify if that environment is supported.

Additional

Based on the current state of Internet Explorer (IE) and when SDK v2.x will be available, I would propose that IE would be an explicit exclusion (list as not supported). So that (if) anyone is still attempting to support IE, it tells them that any issues will be up to them to address with polyfills (if possible) etc

MSNev commented 10 months ago

Current think will be that the initial base ES level should be ES2020, so any "older" browsers that don't support at least this level will not be supported.

Open to suggestions on any higher level that we should support (don't really want to go lower as by the time V2 is out it will be several years old and anyone needing to support the older version can continue using v1 in it's experiential state)

AbhiPrasad commented 8 months ago

I heavily recommend OpenTelemetry instrumentation tries to support as many versions of ES as possible. We don't necessarily have the same security concerns with browser support as we do with something like minimum node version or runtime version, and it feels really bad when instrumentation blocks you from deploying code as an application developer.

Although there is a workaround available for users (using babel/adding polyfills), it does introduce more friction points and makes bundlerless setups less viable. There are also many browser-based runtimes like smart TVs or desktop application web-views where it is really difficult to upgrade browser versions.

I would recommend we adopt ES2018. With ES2018 we get async/await, .includes(), Object.values(), Promise.finally and object rest/spread properties. This means you can write relatively modern JS code, save a lot of bundle size as well with the utilities exposed, and write more performant code at times with usage of async generators and async/await.

ES2019 mostly adds utilities like Array.flatMap and Object.fromEntries, which doesn't seem to add too much value to existing instrumentation. ES2020 does add nullish coalescing, globalThis, and dynamic import() which does have solid benefits, but doesn't seem too groundbreaking in terms of improving instrumentation. Things like import.meta and dynamic import() are often taken care of by bundlers like vite or webpack as well.

pichlermarc commented 7 months ago

@MSNev I just realized that we don't have a short summary on the issue letting people know why we're doing this. Would you mind adding it? I think it can be helpful in moving the discussion forward if others could read up on it. :slightly_smiling_face:

@AbhiPrasad I'm not at all a browser expert so this is likely a naive view so I'm just trying to gather some info to chew on right now :slightly_smiling_face:

So a few questions:

AbhiPrasad commented 7 months ago

Isn't it rather common to have tooling set up for sites already the polyfill and transpile their code if needed?

Yes, but there are a certain percentage of devs who want to use bundleless setups with ESM straight up (no transforms/polyfills). Many people also consume directly from https://unpkg.com/ for example. I'm proposing we try to maximize compatibility for them.

Of course this is a maintenance tradeoff, so I can see the arguments against my reasoning above as well.

Let's say we choose ES2020 as the base ES level, and we want to use things like BigInt: from your point of view is there anything that we could do that would smooth that over for people who do/do not have tooling like this set up already? How much work would it be for people to do?

People would have to adjust their bundler/polyfill generator to accomodate this new setting. This really depends on the setup of someone's configuration, this can really vary across the JavaScript ecosystem.

The nice thing of targeting an ecmascript version instead of arbitrary browser versions is that we can clearly list supported browsers alongside expected JS features used by the OTEL SDKs. Then we can just add a list of this to the docs.

How do other commonly used web libraries/frameworks usually handle this?

Most recommend using https://github.com/zloirock/core-js or setting up a https://github.com/browserslist/browserslist that bundlers like vite/webpack can configure. Next.js is a good example: https://nextjs.org/docs/architecture/supported-browsers

TypeScript picked es2018 for their 5.0: https://github.com/microsoft/TypeScript/pull/51387, but that was because of attempts to maximize API with Node 14. If Node 14 is being kept for the node side of things, this may be useful for contributors (standard set of ES features used across the repo).

NickBolles commented 4 months ago

FWIW we recently were starting to implement OTEL and ran into browser compatibility issues. Specifically zone.js 13 drops support for some older browser versions (<IOS 16): https://github.com/angular/angular/issues/54867

this isn't something that's easy to find without getting a bug report for it or doing extensive and expensive browser testing on tons of different devices. Thankfully a customer caught it in our demo environment. We are looking to transpile zone.js or pin the version to restore support to these older browsers.

DocuSigns browser support matrix is surprisingly very similar to next.js's browser support referred to above, if that helps with guiding the decision

MSNev commented 2 months ago

Notes on why I'm still thinking that the minimum should be no smaller than (or maybe higher) ES2020

Based on nextjs browser matrix

ES6 does not provide support for several used functions, so they would already require polyfills

Node versions

MSNev commented 4 weeks ago

I believe we can close this issue now