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(resources): implements service.instance.id #4608

Merged
merged 12 commits into from
Apr 15, 2024

Conversation

maryliag
Copy link
Contributor

@maryliag maryliag commented Apr 5, 2024

Which problem is this PR solving?

Implements service.instance.id

Short description of the changes

From the Semantic Convention on Service, we have the values of service.name as required, as service.instance.id, service.version and service.namespace as recommended.
We are not able to set the value of version and namespace by ourselves, but following the semantic convention for service.instance.id, the value can be generated using a randomUUID (we have a similar implementation of this in other SDKs, such as the Java one).
This commit adds a value for service.instance.id by default, which can still be overwritten by the environment variable or cloud provider.

This commit also updates some deprecated strings and add missing sdk.shutdown to all sdk.start on the sdk test file.

How Has This Been Tested?

  • Added test on sdk.test
  • Tested manually

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added

@maryliag maryliag requested a review from a team April 5, 2024 14:31
@maryliag maryliag force-pushed the implement-serviceid branch 2 times, most recently from ef5ba1f to 059622b Compare April 5, 2024 14:47
Copy link

codecov bot commented Apr 5, 2024

Codecov Report

Merging #4608 (8628f8a) into main (c046867) will decrease coverage by 2.06%.
Report is 6 commits behind head on main.
The diff coverage is 80.00%.

❗ Current head 8628f8a differs from pull request most recent head 54baf54. Consider uploading reports for the commit 54baf54 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4608      +/-   ##
==========================================
- Coverage   92.83%   90.77%   -2.06%     
==========================================
  Files         329       90     -239     
  Lines        9528     1930    -7598     
  Branches     2053      417    -1636     
==========================================
- Hits         8845     1752    -7093     
+ Misses        683      178     -505     
Files Coverage Δ
packages/opentelemetry-resources/src/Resource.ts 100.00% <100.00%> (ø)
.../platform/browser/ServiceInstanceIDDetectorSync.ts 100.00% <100.00%> (ø)
...lemetry-resources/src/detectors/EnvDetectorSync.ts 94.11% <50.00%> (ø)

... and 242 files with indirect coverage changes

@trentm
Copy link
Contributor

trentm commented Apr 5, 2024

Thanks for this!

I also consider adding this to the default SDK value, but that would unnecessarily force a call for a randomUUID creation on cases that are not required, so I opted for its creation only when not value is set during the environment detector checks.

I kinda think it should be on the default Resource (

static default(): IResource {
return new Resource({
[SemanticResourceAttributes.SERVICE_NAME]: defaultServiceName(),
[SemanticResourceAttributes.TELEMETRY_SDK_LANGUAGE]:
SDK_INFO[SemanticResourceAttributes.TELEMETRY_SDK_LANGUAGE],
[SemanticResourceAttributes.TELEMETRY_SDK_NAME]:
SDK_INFO[SemanticResourceAttributes.TELEMETRY_SDK_NAME],
[SemanticResourceAttributes.TELEMETRY_SDK_VERSION]:
SDK_INFO[SemanticResourceAttributes.TELEMETRY_SDK_VERSION],
});
}
), but I'm curious for other's opinions.

I think it is a bit odd that one would only get service.instance.id if using the EnvDetector because this default service.instance.id doesn't have anything to do with process.env. values.

I don't think the extra crypto.randomUUID() call is a real perf concern. This suggests it takes on the order of 0.1 microseconds on my machine:

% cat timeuuid.js
const { randomUUID } = require('crypto');
const ben = require('ben');
const ms = ben(1e6, function () {
    randomUUID();
});
console.log('crypto.randomUUID(): %dms per iteration', ms);

% node timeuuid.js
crypto.randomUUID(): 0.000105ms per iteration

% node timeuuid.js
crypto.randomUUID(): 0.000095ms per iteration

% node timeuuid.js
crypto.randomUUID(): 0.00009ms per iteration

It would only be called once per Resource.default() call, which is only once in a basic usage of NodeSDK.

@maryliag
Copy link
Contributor Author

maryliag commented Apr 5, 2024

ah good to hear it doesn't cause such a perf impact!
The place you suggested is exactly the place I tried first. I can add there, just a heads up that it breaks a lot of tests, so the PR will be quite big to fix all places, but hopefully it's easy to review 😄

@trentm
Copy link
Contributor

trentm commented Apr 5, 2024

just a heads up that it breaks a lot of tests, so the PR will be quite big

Thanks for slogging through those.

@maryliag
Copy link
Contributor Author

maryliag commented Apr 5, 2024

Implementation and PR description updates. I will focus on the tests now 😄

@maryliag maryliag force-pushed the implement-serviceid branch 2 times, most recently from f6b4645 to fde7eee Compare April 9, 2024 21:09
packages/opentelemetry-resources/src/Resource.ts Outdated Show resolved Hide resolved
packages/opentelemetry-resources/src/Resource.ts Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@maryliag maryliag force-pushed the implement-serviceid branch 2 times, most recently from 6eb66ea to 745add0 Compare April 10, 2024 13:07
@maryliag maryliag changed the title feat(node-sdk): implements service.instance.id feat(resources): implements service.instance.id Apr 10, 2024
@pichlermarc
Copy link
Member

Short summary of what we discussed in the SIG Meeting today 🙂

  • we'll move the detection of this attribute to it's own resource detector in the @opentelemetry/resources package.
  • people that currently set up their SDKs manually can use that new resource detector in the setup code (this is their opt-in)
  • we add the detector in NodeSDK if an env var is set AND no list of resource detectors is passed via the resourceDetectors constructor option
    • we can use OTEL_NODE_RESOURCE_DETECTORS for this (which contains a comma seperated list of resource detectors to turn on), the values for existing resource detectors should match this list, we'll discuss the value for the new detector on the PR - instance or serviceinstance comes to mind.

Implementing the resource detector and adding the env var behavior in NodeSDK can be done in seperate PRs to speed up the review process. 🚀

Separately from that:

@maryliag maryliag force-pushed the implement-serviceid branch 3 times, most recently from 148d48f to 19c7189 Compare April 10, 2024 18:26
@maryliag
Copy link
Contributor Author

maryliag commented Apr 10, 2024

Latest changes (and some questions):

  1. Created serviceInstanceIDDetector
  2. The Browser version does nothing and the Node version sets the value with randomUUID
  3. The default resource is working the same way. I removed the changes I had made there (just keep the updates on deprecated values)
  4. I added the new detector on the default list of detectors only if the env var OTEL_NODE_EXPERIMENTAL_DEFAULT_SERVICE_INSTANCE_ID is set. The other condition will come on a later PR. Or should I completely remove this part for this PR?

Looking for some help: I added a few tests for a few cases with value coming fro env var VS should get set based on the experimental flag and one of the tests is failing but I don't quite get why.
The test failing is where I don't pass a service.instance.id on env var, but I do set to true the OTEL_NODE_EXPERIMENTAL_DEFAULT_SERVICE_INSTANCE_ID, so I should see a random UUID for its value, but on the test itself, it's returning undefined. When I print here the value of resources, I don't see anything getting set for service.instance.id. This problem is not particular to the implementation of using the detectors, because I was having the same error when setting the value on the defaultResources

@maryliag
Copy link
Contributor Author

maryliag commented Apr 10, 2024

while working on the following PR I realize it makes more sense to have all the logic of the env var on the other one, so I remove those from here. This PR focus on the creation of the service instance ID detector. It currently is not called anywhere, but we have a test showcasing that it works as expected if the user initializes their NodeSDK with it.

The following PR (#4626) will add the OTEL_NODE_EXPERIMENTAL_DEFAULT_SERVICE_INSTANCE_ID and OTEL_NODE_RESOURCE_DETECTORS with their respective logics

maryliag added a commit to maryliag/opentelemetry-js that referenced this pull request Apr 10, 2024
Follow up from open-telemetry#4608

Adds the resource detector ServiceInstanceIDDetector on the NodeSDK constructor.
It only gets added by default on any of those conditions:
- the value `serviceinstance` is part of the list `OTEL_NODE_RESOURCE_DETECTORS`
- `OTEL_NODE_EXPERIMENTAL_DEFAULT_SERVICE_INSTANCE_ID` is set to `true`
maryliag added a commit to maryliag/opentelemetry-js that referenced this pull request Apr 10, 2024
Follow up from open-telemetry#4608

Adds the resource detector ServiceInstanceIDDetector on the NodeSDK constructor.
It only gets added by default on any of those conditions:
- the value `serviceinstance` is part of the list `OTEL_NODE_RESOURCE_DETECTORS`
- `OTEL_NODE_EXPERIMENTAL_DEFAULT_SERVICE_INSTANCE_ID` is set to `true`
maryliag added a commit to maryliag/opentelemetry-js that referenced this pull request Apr 10, 2024
Follow up from open-telemetry#4608

Adds the resource detector ServiceInstanceIDDetector on the NodeSDK constructor.
It only gets added by default on any of those conditions:
- the value `serviceinstance` is part of the list `OTEL_NODE_RESOURCE_DETECTORS`
- `OTEL_NODE_EXPERIMENTAL_DEFAULT_SERVICE_INSTANCE_ID` is set to `true`
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Generally looks good 🙂

We can drop the Detector and use DetectorSync only. Other than that, only comments about marking things as @experimental.

Thanks for sticking with us through the review-process 🙂

Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
* feat(node-sdk): implements `service.instance.id`

Implements `service.instance.id`

Signed-off-by: maryliag <[email protected]>

* implement service instance id by default with env var

use an experimental environment variable to set the default value of `service.instance.id` as default

* Update CHANGELOG.md

Co-authored-by: Marc Pichler <[email protected]>

* create separate add experimental for browser and node

create a function to add experimental default values (currently only service instance id, but others can be added in the future),
with the value being set on the node case, but not on the browser case.

Signed-off-by: maryliag <[email protected]>

* use resource detector for service instance id

use a resource detector for service instance id

* remove references to env variables

* remove Detector and use DetectorSync

remove the Detector and use the DetectorSync instead. Also mark things as experimental.

Signed-off-by: maryliag <[email protected]>

* Update packages/opentelemetry-resources/src/platform/node/ServiceInstanceIDDetectorSync.ts

Co-authored-by: Marc Pichler <[email protected]>

* rename ServiceInstanceIDDetector to ServiceInstanceIDDetectorSync

* change ID to Id

* renaming something temp so git will pick up the name change

it was not picking up the case change on previous commit

* update to final name using Id

---------

Signed-off-by: maryliag <[email protected]>
Co-authored-by: Marc Pichler <[email protected]>
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…try#4626)

* feat(sdk-node): add serviceInstanceIDDetector to NodeSDK

Follow up from open-telemetry#4608

Adds the resource detector ServiceInstanceIDDetector on the NodeSDK constructor.
It only gets added by default on any of those conditions:
- the value `serviceinstance` is part of the list `OTEL_NODE_RESOURCE_DETECTORS`
- `OTEL_NODE_EXPERIMENTAL_DEFAULT_SERVICE_INSTANCE_ID` is set to `true`

* remove OTEL_NODE_EXPERIMENTAL_DEFAULT_SERVICE_INSTANCE_ID

Signed-off-by: maryliag <[email protected]>

* update readme on how to use `OTEL_NODE_RESOURCE_DETECTORS`

* feedback from review

* Update experimental/packages/opentelemetry-sdk-node/README.md

Co-authored-by: Marc Pichler <[email protected]>

* feedback from review

---------

Signed-off-by: maryliag <[email protected]>
Co-authored-by: Marc Pichler <[email protected]>
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.

4 participants