Converge sync and async resources #5350
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pichlermarc
reviewed
Jan 17, 2025
Comment on lines
16
to
26
| import * as assert from 'assert'; | ||
| import { ResourceDetectionConfig } from '../../src'; | ||
| import { DetectedResource, ResourceDetector } from '../../src/types'; | ||
|
|
||
| // DO NOT MODIFY THIS DETECTOR: Previous detectors used Resource as IResource did not yet exist. | ||
| // If compilation fails at this point then the changes made are breaking. | ||
| class RegressionTestResourceDetector_1_9_1 implements Detector { | ||
| async detect(_config?: ResourceDetectionConfig): Promise<Resource> { | ||
| return Resource.empty(); | ||
| class RegressionTestResourceDetector_1_9_1 implements ResourceDetector { | ||
| detect(_config?: ResourceDetectionConfig): DetectedResource { | ||
| return {}; | ||
| } | ||
| } |
There was a problem hiding this comment.
This test can actually go now - there's no need to test for compatibility with 1.9.1 anymore in 2.x
Edit: also allows us to drop the dependency on @opentelemetry/[email protected]
pichlermarc
reviewed
Jan 17, 2025
| @@ -17,6 +17,7 @@ import { Suite } from 'mocha'; | |||
| import * as assert from 'assert'; | |||
| import { BROWSER_ATTRIBUTES } from '../src/types'; | |||
| import { IResource } from '@opentelemetry/resources'; | |||
| import { DetectedResource } from '@opentelemetry/resources/src/types'; | |||
There was a problem hiding this comment.
Is this an intentional deep-import? 🤔
|
|
||
| public static EMPTY = new Resource({}); |
There was a problem hiding this comment.
If in the future resources become mutable, would we be able to defend ourselves against this being mutated? 🤔
| return attrs; | ||
| } | ||
|
|
||
| public merge(resource: Resource | null) { |
There was a problem hiding this comment.
Hmm, I think here we may run into a #5283-like situation, we won't be able to merge resources of different package versions.
|
|
||
| /** | ||
| * BrowserDetector will be used to detect the resources related to browser. | ||
| * BrowserDetectorSync will be used to detect the resources related to browser. |
There was a problem hiding this comment.
Suggested change
| * BrowserDetectorSync will be used to detect the resources related to browser. | |
| * BrowserDetector can be used to detect the resources related to browser. |
|
|
||
| /** | ||
| * EnvDetector can be used to detect the presence of and create a Resource | ||
| * EnvDetectorSync can be used to detect the presence of and create a Resource |
There was a problem hiding this comment.
Suggested change
| * EnvDetectorSync can be used to detect the presence of and create a Resource | |
| * EnvDetector can be used to detect the presence of and create a Resource |
|
|
||
| /** | ||
| * OSDetector detects the resources related to the operating system (OS) on | ||
| * OSDetectorSync detects the resources related to the operating system (OS) on |
There was a problem hiding this comment.
Suggested change
| * OSDetectorSync detects the resources related to the operating system (OS) on | |
| * OSDetector detects the resources related to the operating system (OS) on |
| export { ResourceDetectionConfig } from './config'; | ||
| export { detectResources } from './detect-resources'; | ||
| export { | ||
| browserDetector, | ||
| browserDetectorSync, |
There was a problem hiding this comment.
Hmm, is there a reason browserDetectorSync is still here? 🤔
| hostDetector, | ||
| hostDetectorSync, | ||
| noopDetector, |
There was a problem hiding this comment.
We did not have this exported, before - is this on purpose?
Comment on lines
+340
to
+362
| // describe('compatibility', () => { | ||
| // it('should merge resource with old implementation', () => { | ||
| // const resource = Resource.EMPTY; | ||
| // const oldResource = new Resource190({ fromold: 'fromold' }); | ||
|
|
||
| const mergedResource = resource.merge(oldResource); | ||
| // const mergedResource = resource.merge(oldResource); | ||
|
|
||
| assert.strictEqual(mergedResource.attributes['fromold'], 'fromold'); | ||
| }); | ||
| // assert.strictEqual(mergedResource.attributes['fromold'], 'fromold'); | ||
| // }); | ||
|
|
||
| it('should merge resource containing async attributes with old implementation', async () => { | ||
| const resource = new Resource( | ||
| {}, | ||
| Promise.resolve({ fromnew: 'fromnew' }) | ||
| ); | ||
| const oldResource = new Resource190({ fromold: 'fromold' }); | ||
| // it('should merge resource containing async attributes with old implementation', async () => { | ||
| // const resource = new Resource({ | ||
| // attributes: { fromNew: Promise.resolve('fromnew') }, | ||
| // }); | ||
| // const oldResource = new Resource190({ fromold: 'fromold' }); | ||
|
|
||
| const mergedResource = resource.merge(oldResource); | ||
| assert.strictEqual(mergedResource.attributes['fromold'], 'fromold'); | ||
| // const mergedResource = resource.merge(oldResource); | ||
| // assert.strictEqual(mergedResource.attributes['fromold'], 'fromold'); | ||
|
|
||
| await mergedResource.waitForAsyncAttributes?.(); | ||
| assert.strictEqual(mergedResource.attributes['fromnew'], 'fromnew'); | ||
| }); | ||
| }); | ||
| // await mergedResource.waitForAsyncAttributes?.(); | ||
| // assert.strictEqual(mergedResource.attributes['fromnew'], 'fromnew'); | ||
| // }); | ||
| // }); |
There was a problem hiding this comment.
hmm, this looks like a leftover.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes it so that any resource detector can detect sync and async resources, returning both in a single resource object.
attributeskey, 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.