-
Notifications
You must be signed in to change notification settings - Fork 897
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
OTEP: Define ResourceProvider #4316
base: main
Are you sure you want to change the base?
Changes from 1 commit
a830655
c8d5301
a3cb621
2fd655c
a1d4461
844ffa8
cc9877f
3cd7641
7e24b05
6e1d24d
3d01a5f
47d549e
b134f1b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -277,52 +277,101 @@ Attention is placed on making the ResourceProvider thread safe, without introduc | |
any locking or synchronization overhead to `GetResource`, which is the only | ||
ResourceProvider method on the hot path for OpenTelemetry instrumentation. | ||
|
||
``` | ||
```php | ||
// Example of a thread-safe ResourceProvider | ||
class ResourceProvider{ | ||
|
||
*Resource resource | ||
Lock lock | ||
listeners [func(resource){}] // a list of callback functions | ||
Map[string:Entity] entities // a map of Entities using that uses entity IDs as keys | ||
Array[EntityListener] listeners | ||
|
||
GetResource(){ | ||
GetResource() Resource { | ||
return this.resource; | ||
} | ||
|
||
OnChange(listener) { | ||
OnChange(EntityListener listener) { | ||
this.lock.Acquire(); | ||
|
||
listeners.append(listener) | ||
listeners.Append(listener); | ||
|
||
this.lock.Release(); | ||
} | ||
|
||
// MergeResource essentially has the same implementation as SetAttribute. | ||
MergeResource(resource){ | ||
UpdateEntity(string ID, Map[Attribute] attributes){ | ||
this.lock.Acquire(); | ||
|
||
var mergedResource = this.resource.Merge(resource) | ||
// Acquire the correct entity based on ID | ||
var entity = this.entities[ID] | ||
|
||
// If there is no entity, log the error and return. This follows the pattern | ||
// of not returning errors in the OpenTelemetry API. | ||
if(!entity) { | ||
LogError(EntityNotFound); | ||
this.lock.Release(); | ||
return; | ||
} | ||
|
||
// Update the attributes on the entity. | ||
entity.attributes.Merge(attributes); | ||
|
||
// Update the attributes on the resource. | ||
var mergedResource = this.resource.Merge(attributes); | ||
|
||
// safely change the resource reference without blocking | ||
AtomicSwap(this.resource, mergedResource) | ||
AtomicSwap(this.resource, mergedResource); | ||
|
||
// calling listeners inside of the lock ensures that the listeners do not fire | ||
// out of order or get called simultaneously by multiple threads, but would | ||
// also allow a poorly implemented listener to block the ResourceProvider. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, plus the statement above is not really correct since the listener could just enqueue a task in a thread pool. I'm not sure why this part should be specified here. It isn't even required in languages/runtimes that do not have threads or commonly don't use them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes when it comes to language-specific examples it's best to look at prototypes, not pseudo-code. The pseudo-code is helpful to show that there is at least one way to implement the proposed changed in a multi-threaded language. It shouldn't be considered the only way or even the best way, but just something to allow us to discuss potential issues with the design that implementations may need to think about. |
||
for (listener in listeners) { | ||
listener(mergedResource) | ||
for (listener in this.listeners) { | ||
// create an EntityState event from the entity | ||
var entityState = entity.EntityState(); | ||
listener.OnEntityState(entityState, mergedResource); | ||
} | ||
|
||
this.lock.Release(); | ||
|
||
} | ||
|
||
FreezePermanent(){ | ||
DeleteEntity(string ID, Map[Attribute] attributes){ | ||
this.lock.Acquire(); | ||
|
||
this.isFrozen = true; | ||
// Acquire the correct entity based on ID | ||
var entity = this.entities[ID] | ||
|
||
// If there is no entity, log the error and return. This follows the pattern | ||
// of not returning errors in the OpenTelemetry API. | ||
if (!entity) { | ||
LogError(EntityNotFound); | ||
this.lock.Release(); | ||
return; | ||
} | ||
|
||
// remove the entity from the map | ||
this.entities.Delete(ID); | ||
|
||
// Get the attributes to delete from the resource | ||
var keys = new Array(); | ||
for (entity.attributes as name:value) { | ||
keys.Push(name) | ||
} | ||
|
||
// Delete the attributes | ||
var mergedResource = this.resource.Delete(keys); | ||
|
||
this.lock.Release(); | ||
// safely change the resource reference without blocking | ||
AtomicSwap(this.resource, mergedResource); | ||
|
||
// calling listeners inside of the lock ensures that the listeners do not fire | ||
// out of order or get called simultaneously by multiple threads, but would | ||
// also allow a poorly implemented listener to block the ResourceProvider. | ||
for (listener in this.listeners) { | ||
// create an EntityDelete event from the entity | ||
var entityDelete = entity.EntityDelete(); | ||
listener.OnEntityDelete(entityDelete, mergedResource); | ||
} | ||
|
||
this.lock.Release(); | ||
} | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not really understand the pseudo code syntax. It seems to be Go with classes and some strange extra syntax that I do not get. Also, explicit this is really uncommon and only used in languages that require it due to bad language design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make this comment again - The syntax/language for pseudo-code isn't important here.
Are you able to understand what the goal of the interface is from the description and the example? If so, let's evaluate that, not choice of pseudo-code syntax.
If you have specific things you don't understand, list them so they can be addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add that regardless of the pseudo-code someone chooses to use, we request that the examples to be 100% explicit and not assume that the reader knows any implicit details about a particular programming language.