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: add alibaba cloud (aliyun) detector #2378

Closed

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Jul 27, 2021

Which problem is this PR solving?

  • Add resource detector support for Alibaba Cloud (Aliyun).
  • Alibaba Cloud has ~10% of the global market share, ranking third in the world and first in Asia-Pacific. Ref.

Short description of the changes

  • Bootstrap the package with Alibaba Cloud ECS detector.

@codecov
Copy link

codecov bot commented Jul 27, 2021

Codecov Report

Merging #2378 (9f75afe) into main (e089984) will increase coverage by 0.45%.
The diff coverage is 97.61%.

❗ Current head 9f75afe differs from pull request most recent head 87d68b0. Consider uploading reports for the commit 87d68b0 to get more accurate results

@@            Coverage Diff             @@
##             main    #2378      +/-   ##
==========================================
+ Coverage   92.36%   92.81%   +0.45%     
==========================================
  Files         128      147      +19     
  Lines        4244     5263    +1019     
  Branches      867     1071     +204     
==========================================
+ Hits         3920     4885     +965     
- Misses        324      378      +54     
Impacted Files Coverage Δ
...detector-aliyun/src/detectors/AliyunEcsDetector.ts 97.56% <97.56%> (ø)
...ry-resource-detector-aliyun/src/detectors/index.ts 100.00% <100.00%> (ø)
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️
...es/opentelemetry-context-zone-peer-dep/src/util.ts 100.00% <0.00%> (ø)
...ages/opentelemetry-exporter-collector/src/types.ts 100.00% <0.00%> (ø)
...ry-exporter-collector/src/CollectorExporterBase.ts 92.15% <0.00%> (ø)
packages/opentelemetry-web/src/types.ts 100.00% <0.00%> (ø)
packages/opentelemetry-web/src/utils.ts 94.90% <0.00%> (ø)
...-instrumentation-fetch/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)
... and 12 more

@legendecas legendecas force-pushed the feat/aliyun-detector branch from 52cc141 to 87d68b0 Compare July 27, 2021 10:10
@Flarna
Copy link
Member

Flarna commented Jul 27, 2021

I think this should be moved to the contrib repo. I know AWS and GCP is also on this repo but actually they should be also moved.

@legendecas
Copy link
Member Author

legendecas commented Jul 27, 2021

@Flarna do we have any plan to move these packages to the contrib repo? Or maybe I can move them along with the Alibaba Cloud detector together?

Update: found issue #1689. Maybe it's time to do it.

@Flarna
Copy link
Member

Flarna commented Jul 27, 2021

I guess it's just a matter of waiting on someone actually doing it.
Meanwhile versions in core/contrib are quite well aligned so it shouldn't be that hard to avoid double release or releasing of a newer package with a lower release number.

I would not combine your PR with the move as this just complicates the review process.

Lets wait if the maintainers come up with a guideline if/where in contrib the detectors should be located and maybe also regarding the timeline.

@@ -480,6 +480,8 @@ As an alternative, consider setting `faas.id` as a span attribute instead.


export enum CloudProviderValues {
/** Alibaba Cloud. */
ALIYUN = 'aliyun',
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

for now you can just add const for that inside the package you are adding

@obecny
Copy link
Member

obecny commented Jul 27, 2021

you can add new detector here -> https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/detectors/node , moving other detectors - separate PR, I think we should do this yet before GA

@legendecas
Copy link
Member Author

@obecny thanks for the suggestion. I'll do it with separate PRs.

@legendecas legendecas closed this Jul 27, 2021
@legendecas legendecas deleted the feat/aliyun-detector branch August 10, 2021 03:55
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