feature(backend): updating metadata fields to match semantic conventions #2717
Conversation
server/model/spans.go
Outdated
| @@ -9,6 +9,18 @@ import ( | |||
| "go.opentelemetry.io/otel/trace" | |||
| ) | |||
|
|
|||
| type TracetestMetadataField string | |||
There was a problem hiding this comment.
do we really need this type? it seems that all the references to values for this convert it to string, and no other operation is done. Given that this is just a constant to avoid repetition, and it's very unlikely that we'd want to attach methods to it, does it make sense to keep it as a simple "string"?
server/model/spans.go
Outdated
| TracetestMetadataFieldDuration TracetestMetadataField = "tracetest.span.duration" | ||
| TracetestMetadataFieldType TracetestMetadataField = "tracetest.span.type" | ||
| TracetestMetadataFieldName TracetestMetadataField = "tracetest.span.name" | ||
| TracetestMetadataFieldParentID TracetestMetadataField = "tracetest.span.parent_uid" |
There was a problem hiding this comment.
is this field renamed to parent_Uid intentionally or is it a typo?
| TracetestMetadataFieldParentID TracetestMetadataField = "tracetest.span.parent_uid" | |
| TracetestMetadataFieldParentID TracetestMetadataField = "tracetest.span.parent_id" |
There was a problem hiding this comment.
This was definitely a fat finger
server/model/traces.go
Outdated
| @@ -25,7 +25,7 @@ func NewTrace(traceID string, spans []Span) Trace { | |||
|
|
|||
| rootSpans := make([]*Span, 0) | |||
| for _, span := range spanMap { | |||
| parentID := span.Attributes["parent_id"] | |||
| parentID := span.Attributes[string(TracetestMetadataFieldParentID)] | |||
There was a problem hiding this comment.
this values are not the same. the original key is parent_id, but the value defined in the constant is tracetest.span.parent_uid. Is this intentional? Doesn't this changes the key?
| @@ -57,6 +57,7 @@ func (m OpenAPI) Span(in model.Span) openapi.Span { | |||
| EndTime: in.EndTime.UnixMilli(), | |||
| Attributes: attributes, | |||
| Children: m.Spans(in.Children), | |||
| Name: in.Name, | |||
There was a problem hiding this comment.
I just found this bug we have had for a while now haha
| @@ -21,10 +25,24 @@ func (ds AttributeDataStore) Source() string { | |||
| return "attr" | |||
| } | |||
|
|
|||
| func (ds AttributeDataStore) getFromAlias(name string) (string, error) { | |||
There was a problem hiding this comment.
Adding alias check, in this case we only support name for the moment
server/expression/data_store.go
Outdated
| if found { | ||
| value := ds.Span.Attributes.Get(alias) | ||
| if value == "" { | ||
| return "", fmt.Errorf(`attribute "%s" not found`, alias) | ||
| } | ||
|
|
||
| return value, nil | ||
| } |
There was a problem hiding this comment.
I think we can reduce nesting here:
| if found { | |
| value := ds.Span.Attributes.Get(alias) | |
| if value == "" { | |
| return "", fmt.Errorf(`attribute "%s" not found`, alias) | |
| } | |
| return value, nil | |
| } | |
| if !found { | |
| return "", fmt.Errorf(`attribute "%s" not found`, alias) | |
| } | |
| value := ds.Span.Attributes.Get(alias) | |
| if value == "" { | |
| return "", fmt.Errorf(`attribute "%s" not found`, alias) | |
| } | |
| return value, nil |
server/expression/data_store.go
Outdated
| @@ -13,6 +13,10 @@ type DataStore interface { | |||
| Get(name string) (string, error) | |||
| } | |||
|
|
|||
| var AttributeAlias = map[string]string{ | |||
xoscar
deleted the
2713-analyzer-results-fix-validations-for-tracetest-custom-attributes
branch
…ons (#2717) * feature(backend): updating metadata fields to match semantic conventions * feature(backend): PR improvements and aliases for attributes * feature(backend): fixing unit tests * feature(backend): PR improvements * feature(backend): fixing error message
This PR updates the tracetest metadata attributes to match the semantic conventions
Changes
Fixes
Checklist
https://www.loom.com/share/f78af5a0e33f4a1e9e812acb5115c856