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

chore: upgrade for opentelemtry js v0.12.0 #189

Merged
merged 1 commit into from
Oct 29, 2020

Conversation

aabmass
Copy link
Contributor

@aabmass aabmass commented Oct 28, 2020

  • Make sure to open an issue as a bug/issue/feature before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Part of #188

Upgrade OpenTelemetry peer dependencies to v0.12.0.

  • Distribution was removed altogether
  • Some other fixes for slight API changes
  • I think the codecov drop is flaky since I deleted code?

@aabmass aabmass requested review from mayurkale22 and a team October 28, 2020 02:20
@codecov
Copy link

codecov bot commented Oct 28, 2020

Codecov Report

Merging #189 into master will decrease coverage by 0.07%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #189      +/-   ##
==========================================
- Coverage   95.25%   95.18%   -0.08%     
==========================================
  Files          11       11              
  Lines         337      332       -5     
  Branches       67       66       -1     
==========================================
- Hits          321      316       -5     
  Misses         16       16              
Impacted Files Coverage Δ
...emetry-cloud-monitoring-exporter/src/monitoring.ts 89.28% <0.00%> (ø)
...es/opentelemetry-cloud-trace-exporter/src/trace.ts 96.29% <0.00%> (ø)
...lemetry-cloud-monitoring-exporter/src/transform.ts 96.62% <100.00%> (-0.15%) ⬇️
...etry-cloud-trace-propagator/src/CloudPropagator.ts 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48898d5...8b42fd6. Read the comment docs.

@aabmass aabmass marked this pull request as ready for review October 28, 2020 02:38
@@ -140,7 +140,7 @@ export class MetricExporter implements IMetricExporter {
cb(ExportResult.SUCCESS);
}

shutdown(): void {}
async shutdown(): Promise<void> {}
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious, where did this change come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They changed the interface in the SDK: open-telemetry/opentelemetry-js#1439

@@ -130,6 +130,7 @@ describe('transform', () => {
});

it('should drop unknown attribute types', () => {
// @ts-expect-error
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

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 test is asserting that unsupported attribute types (like object here) are dropped, but it creates a TS error since it is not allowed. In this case, we expect the TS error but want to validate the behavior too

@aabmass aabmass merged commit b0e4012 into GoogleCloudPlatform:master Oct 29, 2020
@aabmass aabmass deleted the upgrade-0.12-188 branch October 29, 2020 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants