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

[engsys] remove openTelemetryCommonJs() rollup name exports from dev-tool #17707

Closed
jeremymeng opened this issue Sep 15, 2021 · 5 comments
Closed
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. EngSys This issue is impacting the engineering system.

Comments

@jeremymeng
Copy link
Member

The latest opentelemetry/api package has esm support so packages depending on the latest version don't need the rollup cjs name exports.

  • all packages using the shared one from dev-tool can safely remove it except one

    • communication-chat which indirectly depends on older version of opentelemetry/api from communication-signaling, which is being updated. We could either wait for a new version of communication-chat, or remove the shared one and add a copy for communication-chat before it has a new version.
  • generated management packages has a copy of the function openTelemetryCommonJs() in rollup.config.js from codegen. We want update code generator to move to latest version of core-rest-pipeline and remove openTelemetryCommonJs()

@jeremymeng jeremymeng added Client This issue points to a problem in the data-plane of the library. EngSys This issue is impacting the engineering system. labels Sep 15, 2021
@jeremymeng jeremymeng added this to the [2021] December milestone Sep 15, 2021
@ramya-rao-a
Copy link
Contributor

@jeremymeng All the new mgmt plane packages are using core-rest-pipeline now. Can you check if your findings above still apply?

@jeremymeng
Copy link
Member Author

@ramya-rao-a many packages still contain openTelemetryCommonJs, for example

[omitted]
sdk/servicefabric/arm-servicefabric/rollup.config.js
26:export function openTelemetryCommonJs() {
138:          ...openTelemetryCommonJs()

sdk/devtestlabs/arm-devtestlabs/rollup.config.js
26:export function openTelemetryCommonJs() {
138:          ...openTelemetryCommonJs()

sdk/logic/arm-logic/rollup.config.js
26:export function openTelemetryCommonJs() {
138:          ...openTelemetryCommonJs()

sdk/datalake-analytics/arm-datalake-analytics/rollup.config.js
26:export function openTelemetryCommonJs() {
138:          ...openTelemetryCommonJs()

sdk/batch/arm-batch/rollup.config.js
26:export function openTelemetryCommonJs() {
138:          ...openTelemetryCommonJs()

@ramya-rao-a
Copy link
Contributor

Looks like apart from the generated mgmt plane packages, the only package using openTelemetryCommonJs function is the one for synapse-monitoring, but it uses the one from shared-config.

@jeremymeng Can you review the case for synapse-monitoring and then log an issue in the code generator for the required changes?

@jeremymeng
Copy link
Member Author

Codegen tracking issue: Azure/autorest.typescript#1282

@jeremymeng jeremymeng changed the title [engsys] remove openTelemetryCommonJs() rollup name exports [engsys] remove openTelemetryCommonJs() rollup name exports from dev-tool Jan 15, 2022
jeremymeng added a commit to jeremymeng/azure-sdk-for-js that referenced this issue Jan 15, 2022
It is a workaround for packages that uses older version of core-tracing
libraries. Now it only applies to two packages.  This PR removes the export from
`dev-tool` and have copies of the function in the two communication packages'
test rollup config instead.

Part of Azure#17707
jeremymeng added a commit that referenced this issue Jan 18, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…19866)

It is a workaround for packages that uses older version of core-tracing
libraries. Now it only applies to two packages.  This PR removes the export from
`dev-tool` and have copies of the function in the two communication packages'
test rollup config instead.

Part of #17707
@jeremymeng
Copy link
Member Author

Closing. all remaining open telemetry commonjs workarounds have separate issues to track the removal of them

@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. EngSys This issue is impacting the engineering system.
Projects
None yet
Development

No branches or pull requests

2 participants