Converge sync and async resources #5350

dyladan · 2025-01-17T15:19:05Z

This PR makes it so that any resource detector can detect sync and async resources, returning both in a single resource object.

Internally, the resource now stores all key/value pairs in a list. This means the override order comes from the order of detection, not the order of when the promises are resolved. Rejected promises are skipped. This should make the order of resource detection significantly more obvious to end users. It also paves the way for entities which explicitly require detection priority to be defined by the order in which entity detectors are configured.
Resource detectors now return plain objects instead of new Resource instances. This should improve type compatibility in the future.
Detected resource interface contains an optional attributes key, which all attributes are nested under. This will make it easier to extend for entities in the future by adding a new key to the object.

To be done in follow-up:

packages/opentelemetry-resources/test/regression/existing-detectors-1-9-1.test.ts

pichlermarc

Thank you for working on this 🙂
A quick preliminary review - I'll have a closer look at the implementation details next week :)

experimental/packages/opentelemetry-browser-detector/test/util.ts

packages/opentelemetry-resources/src/Resource.ts

packages/opentelemetry-resources/src/detectors/BrowserDetector.ts

packages/opentelemetry-resources/src/detectors/EnvDetector.ts

packages/opentelemetry-resources/src/detectors/platform/node/OSDetector.ts

packages/opentelemetry-resources/src/index.ts

packages/opentelemetry-resources/test/Resource.test.ts

codecov · 2025-01-22T01:26:07Z

Codecov Report

Attention: Patch coverage is 85.16484% with 27 lines in your changes missing coverage. Please review.

Project coverage is 94.69%. Comparing base (c00f36e) to head (15f54f0).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...lemetry-resources/src/detectors/BrowserDetector.ts	43.75%	9 Missing ⚠️
...ntelemetry-browser-detector/src/BrowserDetector.ts	0.00%	4 Missing ⚠️
...entelemetry-resources/src/detectors/EnvDetector.ts	93.87%	3 Missing ⚠️
...es/opentelemetry-resources/src/detect-resources.ts	80.00%	2 Missing ⚠️
...ntelemetry-resources/src/detectors/NoopDetector.ts	33.33%	2 Missing ⚠️
...tectors/platform/node/ServiceInstanceIdDetector.ts	33.33%	2 Missing ⚠️
packages/opentelemetry-resources/src/Resource.ts	97.87%	1 Missing ⚠️
...tors/platform/browser/ServiceInstanceIdDetector.ts	0.00%	1 Missing ⚠️
...ces/src/detectors/platform/node/ProcessDetector.ts	91.66%	1 Missing ⚠️
...rs/platform/node/machine-id/getMachineId-darwin.ts	66.66%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5350      +/-   ##
==========================================
+ Coverage   94.54%   94.69%   +0.14%     
==========================================
  Files         318      311       -7     
  Lines        8051     7993      -58     
  Branches     1694     1686       -8     
==========================================
- Hits         7612     7569      -43     
+ Misses        439      424      -15

Files with missing lines	Coverage Δ
...imental/packages/opentelemetry-sdk-node/src/sdk.ts	`96.01% <100.00%> (ø)`
...ental/packages/opentelemetry-sdk-node/src/utils.ts	`88.23% <100.00%> (ø)`
...ges/opentelemetry-resources/src/detectors/index.ts	`100.00% <100.00%> (ø)`
...elemetry-resources/src/detectors/platform/index.ts	`100.00% <100.00%> (ø)`
...ources/src/detectors/platform/node/HostDetector.ts	`100.00% <100.00%> (ø)`
...esources/src/detectors/platform/node/OSDetector.ts	`100.00% <100.00%> (ø)`
...try-resources/src/detectors/platform/node/index.ts	`100.00% <100.00%> (ø)`
...ctors/platform/node/machine-id/getMachineId-bsd.ts	`100.00% <100.00%> (ø)`
...ors/platform/node/machine-id/getMachineId-linux.ts	`100.00% <100.00%> (ø)`
...ctors/platform/node/machine-id/getMachineId-win.ts	`93.33% <100.00%> (ø)`
... and 12 more

packages/opentelemetry-resources/src/Resource.ts

packages/opentelemetry-resources/src/detect-resources.ts

packages/opentelemetry-resources/src/Resource.ts

packages/opentelemetry-resources/src/detectors/BrowserDetector.ts

chancancode · 2025-01-22T19:13:30Z

packages/opentelemetry-resources/test/detectors/node/machine-id/getMachineId-bsd.test.ts

@@ -73,7 +73,7 @@ describe('getMachineId on BSD', () => {

    const machineId = await getMachineId();

-    assert.equal(machineId, '');
+    assert.equal(machineId, undefined);


Suggested change

assert.equal(machineId, undefined);

assert.strictEqual(machineId, undefined);

chancancode · 2025-01-22T19:14:02Z

packages/opentelemetry-resources/test/detectors/node/machine-id/getMachineId-darwin.test.ts

@@ -80,7 +80,7 @@ describe('getMachineId on Darwin', () => {

    const machineId = await getMachineId();

-    assert.equal(machineId, '');
+    assert.equal(machineId, undefined);


Suggested change

assert.equal(machineId, undefined);

assert.strictEqual(machineId, undefined);

chancancode · 2025-01-22T19:14:35Z

packages/opentelemetry-resources/test/detectors/node/machine-id/getMachineId-linux.test.ts

@@ -60,7 +60,7 @@ describe('getMachineId on linux', () => {

    const machineId = await getMachineId();

-    assert.equal(machineId, '');
+    assert.equal(machineId, undefined);


Suggested change

assert.equal(machineId, undefined);

assert.strictEqual(machineId, undefined);

chancancode · 2025-01-22T19:19:01Z

packages/opentelemetry-resources/src/detectors/EnvDetector.ts

-class EnvDetector implements Detector {
+class EnvDetector implements ResourceDetector {
+  // Type, attribute keys, and attribute values should not exceed 256 characters.
+  private readonly _MAX_LENGTH = 255;


nit but is there a reason to prefer to have these as private readonly fields, as opposed to un-exported module-level constant? the latter seems more "truly" private, easier to type, and more mangle/tree-shaking friendly.

no. The implementation was unchanged in this PR but can be fixed in a follow-up. the reason git thinks i changed the impl is because there used to be a different file for the sync detector.

chancancode · 2025-01-23T21:35:03Z

packages/opentelemetry-resources/src/Resource.ts

+    });
+  }
+
+  public get asyncAttributesPending() {


Suggested change

public get asyncAttributesPending() {

public get asyncAttributesPending(): boolean {

chancancode · 2025-01-23T21:35:46Z

packages/opentelemetry-resources/src/Resource.ts

+    return attrs;
+  }
+
+  public getRawAttributes() {


Suggested change

public getRawAttributes() {

public getRawAttributes(): RawResourceAttribute[] {

chancancode · 2025-01-23T21:39:40Z

packages/opentelemetry-resources/src/detectors/BrowserDetector.ts

-  detect(config?: ResourceDetectionConfig): Promise<IResource> {
-    return Promise.resolve(browserDetectorSync.detect(config));
+class BrowserDetector implements ResourceDetector {
+  detect(config?: ResourceDetectionConfig) {


: DetectedResource ...if we want to be consistent about public interface return types, I'd leave a suggestion but this requires adding the import too

chancancode · 2025-01-23T21:40:12Z

packages/opentelemetry-resources/src/detectors/EnvDetector.ts

  /**
   * Returns a {@link Resource} populated with attributes from the
   * OTEL_RESOURCE_ATTRIBUTES environment variable. Note this is an async
   * function to conform to the Detector interface.
   *
   * @param config The resource detection config
   */
-  detect(config?: ResourceDetectionConfig): Promise<IResource> {
-    return Promise.resolve(envDetectorSync.detect(config));
+  detect(_config?: ResourceDetectionConfig) {


: DetectedResource ...if we want to be consistent about public interface return types, I'd leave a suggestion but this requires adding the import too

chancancode · 2025-01-23T21:40:34Z

packages/opentelemetry-resources/src/detectors/NoopDetector.ts

-  detect(): Promise<IResource> {
-    return Promise.resolve(noopDetectorSync.detect());
+export class NoopDetector implements ResourceDetector {
+  detect() {


: DetectedResource ...if we want to be consistent about public interface return types, I'd leave a suggestion but this requires adding the import too

chancancode · 2025-01-23T21:41:40Z

packages/opentelemetry-resources/src/detectors/platform/node/OSDetector.ts

-  detect(_config?: ResourceDetectionConfig): Promise<IResource> {
-    return Promise.resolve(osDetectorSync.detect(_config));
+class OSDetector implements ResourceDetector {
+  detect(_config?: ResourceDetectionConfig) {


: DetectedResource ...if we want to be consistent about public interface return types, I'd leave a suggestion but this requires adding the import too

chancancode · 2025-01-23T21:42:30Z

packages/opentelemetry-resources/src/detectors/platform/node/ProcessDetector.ts

-  detect(config?: ResourceDetectionConfig): Promise<IResource> {
-    return Promise.resolve(processDetectorSync.detect(config));
+class ProcessDetector implements ResourceDetector {
+  detect(_config?: ResourceDetectionConfig) {


: DetectedResource ...if we want to be consistent about public interface return types, I'd leave a suggestion but this requires adding the import too

chancancode

Take it with an unhealthy amount of salt, but other than the suggestions I left (feel free to make your own judgment call), it looks good to me!

trentm

I think this LGTM.

I asked a Q at #5358 (comment)
I'm not totally clear on what the user's experience with resources (usage with the SDK I mean) will be after these changes at #5358, especially when Entities come along.

trentm · 2025-01-23T01:48:20Z

packages/opentelemetry-resources/src/Resource.ts

+    return Resource.FromAttributeList([
+      ...resource._rawAttributes,
+      ...this._rawAttributes,
+    ]);


Might be nice to ahve a comment here that resource is first here because (a) the implementation is "first one wins" and (b) the spec says:

https://opentelemetry.io/docs/specs/otel/resource/sdk/#merge

If a key exists on both the old and updating resource, the value of the updating resource MUST be picked (even if the updated value is empty).

I'll add a comment, but does this implementation convey intent better to you?

return Resource.FromAttributeList( [...this.getRawAttributes(), ...resource.getRawAttributes()].reverse() );

advantages:

More explicit intent

reversing both arrays guards against arrays with duplicate entries (unlikely/impossible but /shrug)

disadvantages:

slightly less efficient

Another possibility would be to change the impl of FromAttributeList to have the opposite order semantic.

edit: actually the first-wins semantic comes from the attributes getter, so that's what would have to change.

package-lock.json

dyladan added 3 commits January 16, 2025 13:29

WIP sync and async resource merge

0ac450e

Fix isPromiseLike

49ef5ed

Fix tests - TODO: attribute fallback order

29ccd0f

dyladan requested a review from a team as a code owner January 17, 2025 15:19

pichlermarc reviewed Jan 17, 2025

View reviewed changes

packages/opentelemetry-resources/test/regression/existing-detectors-1-9-1.test.ts Outdated Show resolved Hide resolved

pichlermarc reviewed Jan 17, 2025

View reviewed changes

dyladan added 7 commits January 21, 2025 16:09

Fix resource usages for 2.x

767947c

Fix remaining tests

9aba287

Unskip tests

9f231aa

Remove deep export

13f2064

Review comments

2fa42c8

Remove noop detector export

ff5af1a

Merge remote-tracking branch 'origin/main' into resource-async-sync

8e513e4

dyladan added 3 commits January 21, 2025 20:26

Lint and cleanup

8236217

Changelog

36d6b9c

Changelog

c1f6d24

chancancode reviewed Jan 22, 2025

View reviewed changes

packages/opentelemetry-resources/src/Resource.ts Outdated Show resolved Hide resolved

chancancode reviewed Jan 22, 2025

View reviewed changes

packages/opentelemetry-resources/src/Resource.ts Outdated Show resolved Hide resolved

chancancode reviewed Jan 22, 2025

View reviewed changes

packages/opentelemetry-resources/src/detect-resources.ts Outdated Show resolved Hide resolved

chancancode reviewed Jan 22, 2025

View reviewed changes

packages/opentelemetry-resources/src/Resource.ts Outdated Show resolved Hide resolved

chancancode reviewed Jan 22, 2025

View reviewed changes

packages/opentelemetry-resources/src/detectors/BrowserDetector.ts Outdated Show resolved Hide resolved

chancancode reviewed Jan 22, 2025

View reviewed changes

dyladan added 4 commits January 23, 2025 09:50

Use shared empty resource

1de27ba

Make asyncAttributesPending readonly

20d297d

Add explicit return type to resource merge

6d1f3a0

Fix types and lint

f13ba2c

Create new resource from list

15f54f0

chancancode reviewed Jan 23, 2025

View reviewed changes

chancancode approved these changes Jan 23, 2025

View reviewed changes

trentm approved these changes Jan 23, 2025

View reviewed changes

dyladan added 5 commits January 27, 2025 09:21

Explict return types

e6dce6b

revert package lock changes

7a93c59

Merge remote-tracking branch 'origin/main' into resource-async-sync

0467bc8

Revert package lock changes

fd4bab8

Use strict equal

9c74732

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converge sync and async resources #5350

Converge sync and async resources #5350

dyladan commented Jan 17, 2025 •

edited

Loading

pichlermarc left a comment

codecov bot commented Jan 22, 2025 •

edited

Loading

chancancode Jan 22, 2025

chancancode Jan 22, 2025

chancancode Jan 22, 2025

chancancode Jan 22, 2025

dyladan Jan 27, 2025

chancancode Jan 23, 2025

chancancode Jan 23, 2025

chancancode Jan 23, 2025

chancancode Jan 23, 2025

chancancode Jan 23, 2025

chancancode Jan 23, 2025

chancancode Jan 23, 2025

chancancode left a comment

trentm left a comment

trentm Jan 23, 2025

dyladan Jan 27, 2025 •

edited

Loading

dyladan Jan 27, 2025 •

edited

Loading

	assert.equal(machineId, undefined);
	assert.strictEqual(machineId, undefined);

	public get asyncAttributesPending() {
	public get asyncAttributesPending(): boolean {

	public getRawAttributes() {
	public getRawAttributes(): RawResourceAttribute[] {

Converge sync and async resources #5350

Are you sure you want to change the base?

Converge sync and async resources #5350

Conversation

dyladan commented Jan 17, 2025 • edited Loading

pichlermarc left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 22, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chancancode left a comment

Choose a reason for hiding this comment

trentm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

dyladan Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

dyladan commented Jan 17, 2025 •

edited

Loading

codecov bot commented Jan 22, 2025 •

edited

Loading

dyladan Jan 27, 2025 •

edited

Loading

dyladan Jan 27, 2025 •

edited

Loading