-
Notifications
You must be signed in to change notification settings - Fork 43
Allow attributes to be identifying or descriptive. #735
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
base: main
Are you sure you want to change the base?
Conversation
@@ -688,6 +693,8 @@ | |||
#[cfg(test)] | |||
mod tests { | |||
use std::error::Error; | |||
use std::fs::{File, OpenOptions}; |
Check failure
Code scanning / clippy
unused import: File Error
@@ -688,6 +693,8 @@ | |||
#[cfg(test)] | |||
mod tests { | |||
use std::error::Error; | |||
use std::fs::{File, OpenOptions}; | |||
use std::io::Write; |
Check failure
Code scanning / clippy
unused import: std::io::Write Error
@@ -793,23 +809,37 @@ | |||
) | |||
.expect("Failed to deserialize expected attribute catalog"); | |||
|
|||
// Write observed output. | |||
let observed_attr_catalog_file = OpenOptions::new() | |||
.create(true) |
Check failure
Code scanning / clippy
file opened with create, but truncate behavior not defined Error
// Check that the resolved registry matches the expected registry. | ||
let expected_registry: Registry = serde_json::from_reader( | ||
std::fs::File::open(format!("{}/expected-registry.json", test_dir)) | ||
.expect("Failed to open expected registry"), | ||
) | ||
.expect("Failed to deserialize expected registry"); | ||
|
||
// Write observed output. | ||
let observed_registry_file = OpenOptions::new() | ||
.create(true) |
Check failure
Code scanning / clippy
file opened with create, but truncate behavior not defined Error
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.
Made 2 minor comments, otherwise LGTM
@@ -87,6 +89,10 @@ pub struct Attribute { | |||
/// Note: This is only used in a telemetry schema specification. | |||
#[serde(skip_serializing_if = "Option::is_none")] | |||
pub value: Option<Value>, | |||
/// Whether the attribute is identifying or descriptive. |
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.
Is there a document describing the difference between these two roles that we could reference here?
// Write observed output. | ||
let observed_attr_catalog_file = OpenOptions::new() | ||
.create(true) | ||
.write(true) | ||
.open(observed_output_dir.join("attribute-catalog.json")) | ||
.expect("Failed to open observed output file"); | ||
serde_json::to_writer_pretty(observed_attr_catalog_file, &observed_attr_catalog) | ||
.expect("Failed to write observed ouptut."); | ||
// Compare values |
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.
What is the purpose of this? Debugging?
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.
no, updating the test values for some of these really large files is easier to look at the new and validate, then copy over.
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 see. Thanks
This is the next phase of open-telemetry/semantic-conventions#2246.
What this does:
role
field to Attribute with optionsIdentifying
orDescriptive
role
changesWhat this does not do:
role
.Why role
Why do we call this
role
? I did some investigation into names and actually thought LLM captured some considerations well, here's (snipped) output: