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

feat: bumped aws components to 1.0 #658

Merged
merged 12 commits into from
Oct 22, 2021

Conversation

willarmiros
Copy link
Contributor

Short description of the changes

  • Bumps AWS X-Ray ID generator, propagator, and resource detector packages to 1.0 since no breaking changes are anticipated.
  • Will leave in draft until core SDK reaches 1.0 first.

Copy link
Contributor

@NathanielRN NathanielRN left a comment

Choose a reason for hiding this comment

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

🎉

@codecov
Copy link

codecov bot commented Sep 8, 2021

Codecov Report

Merging #658 (e10989c) into main (77c215b) will increase coverage by 0.89%.
The diff coverage is n/a.

❗ Current head e10989c differs from pull request most recent head ca124dd. Consider uploading reports for the commit ca124dd to get more accurate results

@@            Coverage Diff             @@
##             main     #658      +/-   ##
==========================================
+ Coverage   96.87%   97.77%   +0.89%     
==========================================
  Files          11       29      +18     
  Lines         640     1436     +796     
  Branches      126      195      +69     
==========================================
+ Hits          620     1404     +784     
- Misses         20       32      +12     
Impacted Files Coverage Δ
...ource-detector-aws/src/detectors/AwsEc2Detector.ts 97.91% <0.00%> (ø)
...y-id-generator-aws-xray/src/platform/node/index.ts 100.00% <0.00%> (ø)
...erator-aws-xray/src/internal/xray-id-generation.ts 100.00% <0.00%> (ø)
...ector-aws/test/detectors/AwsLambdaDetector.test.ts 100.00% <0.00%> (ø)
...propagator-aws-xray/test/AWSXRayPropagator.test.ts 100.00% <0.00%> (ø)
...ource-detector-aws/src/detectors/AwsEcsDetector.ts 100.00% <0.00%> (ø)
...emetry-id-generator-aws-xray/src/platform/index.ts 100.00% <0.00%> (ø)
...ce-detector-aws/src/detectors/AwsLambdaDetector.ts 100.00% <0.00%> (ø)
...detector-aws/test/detectors/AwsEcsDetector.test.ts 100.00% <0.00%> (ø)
...r-aws-xray/src/platform/node/AWSXRayIdGenerator.ts 100.00% <0.00%> (ø)
... and 8 more

@dyladan
Copy link
Member

dyladan commented Sep 9, 2021

This is in conflict with the current lerna.json which claims all packages to be the same version. I think this also has to wait until the release automation is updated to allow for releasing packages with independent versions.

The problems we had with release-please in core generally don't apply to this repo since modules do not depend on each other so it should be possible to use release-please here without issue. If you would like to take a crack at porting the PR I did in core to this repo it would be quite helpful.

@willarmiros
Copy link
Contributor Author

This is in conflict with the current lerna.json which claims all packages to be the same version

Makes sense, I will leave it in draft until the release-please automation is in place and close if it's no longer necessary. I wasn't sure if release-please would be capable of automatically bumping to 1.0.0, since it's at 0.24.0 right now and even if we tagged a change as breaking, I don't know if that would bump the major or minor version.

If you would like to take a crack at porting the PR I did in core to this repo it would be quite helpful.

Sure, we're talking about open-telemetry/opentelemetry-js#2431 just to confirm right? I'll see if I have the bandwidth.

@willarmiros
Copy link
Contributor Author

I wasn't sure if release-please would be capable of automatically bumping to 1.0.0

I guess this answers my question: https://github.com/open-telemetry/opentelemetry-js/blob/5fefbb274893a93033ef3786f75173c62682ade7/release-please-config.json#L4

So I think this PR will still be needed even after adding release-please.

@willarmiros
Copy link
Contributor Author

Opened #659 to add release-please to this repo.

@willarmiros willarmiros marked this pull request as ready for review October 6, 2021 22:14
@willarmiros willarmiros requested a review from a team October 6, 2021 22:14
@willarmiros
Copy link
Contributor Author

@dyladan PTAL

I reverted the manual package.json version bumps and instead just made small changes to each of the 3 target package's readmes. Instead, for each commit that modified a package, I included a Release-As: 1.0.0 message for the commit, per the release-please docs: https://github.com/googleapis/release-please#how-do-i-change-the-version-number

Not sure if this will work since we're targeting several packages unlike the docs, but I think as long as we maintain the Release-As messages in the squash commit body it should do something.

@Flarna
Copy link
Member

Flarna commented Oct 7, 2021

Is it intended that v1.0 of these packages depends on 0.25.0 of core instead 1.0.0?

@willarmiros
Copy link
Contributor Author

@Flarna good catch thank you, updated to SDK 1.0.0

@vmarchaud vmarchaud requested a review from dyladan October 9, 2021 13:44
@willarmiros
Copy link
Contributor Author

@dyladan any chance this could be merged to include in the next release?

Copy link
Member

@rauno56 rauno56 left a comment

Choose a reason for hiding this comment

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

I don't see the initial changes that bumped the package, API and SDK versions anymore - updated in upstream?

@@ -38,16 +38,15 @@ const tracerProvider = new NodeTracerProvider(tracerConfig);

Example trace ID format: 58406520a006649127e371903a2de979

A trace ID consists of two parts; the timestamp and the unique identifier.
A trace ID consists of two parts: the timestamp and the unique identifier.

#### Time Stamp
Copy link
Member

Choose a reason for hiding this comment

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

I know changing the docs wasn't the intention of the PR, but it caught my eye since you did changes in the readme file as well. 😆

Suggested change
#### Time Stamp
#### Timestamp

Copy link
Member

Choose a reason for hiding this comment

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

Since you were at it, I'd actually just format the whole section to be more concise:


A trace ID consists of two parts:

  1. Timestamp: The first 8 hexadecimal digits represent the time of the original request in Unix epoch time. For example, 10:00 AM December 1st, 2016 PST in epoch time is 1480615200 seconds, or 58406520 in hexadecimal digits.
  2. Unique Identifier: The last 24 hexadecimal digits is an random identifier for the trace.

But that comes to style more than correctness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the guidance, I updated it as such! I was not a fan of the old formatting either.

@willarmiros
Copy link
Contributor Author

@dyladan Can this be merged?

@willarmiros willarmiros changed the title chore: bumped aws components to 1.0 feat: bumped aws components to 1.0 Oct 22, 2021
@willarmiros
Copy link
Contributor Author

@obecny @rauno56 any chance you could merge this in when ready?

@obecny obecny merged commit 44d21fe into open-telemetry:main Oct 22, 2021
@chrskrchr
Copy link

👋 Howdy! Any idea of when we can expect a new release of the packages touched in this PR? My organization is sitting on a PR to update to the @@opentelemetry/core@1.0.0 release, and the id-generator-aws-xray is the last package that has yet to be updated to @opentelemetry/core@1.0.0.

We haven't actually observed any strange behavior when packages use different versions of the @otel/core lib, but it sounds like the risk for unexpected behavior here is high:

#695 (comment)

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.

7 participants