Exemplar: Add new record APIs that take exemplar attachments and SpanContext key.#1075
Exemplar: Add new record APIs that take exemplar attachments and SpanContext key.#1075songy23 wants to merge 5 commits intocensus-instrumentation:masterfrom
Conversation
|
|
||
| // Exemplars keys. | ||
| const ( | ||
| AttachmentKeySpanContext = "SpanContext" |
There was a problem hiding this comment.
Is there a reason why this is public?
There was a problem hiding this comment.
This key will be used when 1. record with span context attachment and 2. exporters extract span context from exemplar. If it's not exposed, users may record span context attachment with a random key that exporters don't expect.
In Java it's also public: https://github.com/census-instrumentation/opencensus-java/blob/70f9701a32dee5d2cd07c7d4a1eb82cb2c26b863/contrib/exemplar_util/src/main/java/io/opencensus/contrib/exemplar/util/ExemplarUtils.java#L36.
89e7e97 to
6b8ee3a
Compare
stats/record.go
Outdated
|
|
||
| // RecordWithAttachments records measurements and the given exemplar attachments against context. | ||
| // If there are any tags in the context, measurements will be tagged with them. | ||
| func RecordWithAttachments(ctx context.Context, attachments map[string]interface{}, ms ...Measurement) { |
There was a problem hiding this comment.
I would do something like
type Attachments map[string]interface{}Not sure if interface{} is the right type here, we need something that converts (allows user to convert to String).
There was a problem hiding this comment.
Updated the API to take metricdata.Attachments. I think we can do type assertions with interface{} for the common types that we already knew, for example:
for k, v := range(attachments) {
if spanctx, ok := v.(SpanContext); ok {
//...
} else if str, ok := v.(string); ok {
// ...
} else {
str := fmt.Sprintf("%v", v)
// ...
}
}WDYT?
There was a problem hiding this comment.
Chatted offline, we can instead make the exemplar attachment similar to span attributes. Like:
type Attachment {
key string
value interface{}
}
func SpanContextAttachment(key string, value SpanContext) Attachment {
return Attachment{key: key, value: value}
}
func StringAttachment(key string, value string) Attachment {
return Attachment{key: key, value: value}
}In this way we restricted the type of attachment we want. However at the same time we'll introduce tracing dependency to metrics. I'm not sure if this is the best practice in Go either. @rakyll any suggestions here?
There was a problem hiding this comment.
for non-string attachement is the expectation that they provide string form of the attachment?
If so then you could have
type GetValue interface {
ToString() string
}
type Attachment struct {
key string
value interface{}
}
func NonStringAttachment(key string, value GetValue) *Attachment {
return &Attachment{key: key, value: value}
}
func StringAttachment(key, value string) *Attachment {
return &Attachment{key: key, value: value}
}
func (a *Attachment) encodeAttachement() {
switch v := a.value.(type) {
case GetValue:
fmt.Printf("key: %s, value %s\n", a.key, v.ToString());
case string:
fmt.Printf("key: %s, value %s\n", a.key, v);
default:
panic("unsupported type")
}
}There was a problem hiding this comment.
for non-string attachement is the expectation that they provide string form of the attachment?
IMO no. In the exemplar attachment API we want to keep the value structs as-is, and only do the interpretation/formatting when exporting. For example we want to keep the whole SpanContext struct as the attachment value instead of only keeping a string representation of it. Only when we're about to export to Stackdriver, we get the trace and span id from span context and convert them into strings.
There was a problem hiding this comment.
Chatted offline, we can instead make the exemplar attachment similar to span attributes.
In this way we restricted the type of attachment we want.
Did some experiments on this, actually we won't be able to achieve this goal, because the key and value of exemplar attachment has to be public in order for other packages to access them. #1085 implemented this idea, though I'm more inclined to keep the current API as-is.
c2b600d to
f9f87bd
Compare
|
Going to create a new PR to the Superseded by #1123 |
Updates #1058.
Existing record APIs will continue to work.