Question: Why tag value has to be string?

ashi009 commented 6 years ago

In practice, a tag's value may come from data sources of many different types. For instance, a tag for http-response-status is an int, snapshot-ctime is datetime, and batch-completed is boolean.

The current spec limits a tag's value to a string, which seems to be less ideal. As this forces stats to be designed in a way that's unnatural, and even error-prone pattern (eg. use seconds since epoch or an RFC3339 string to represent a datetime).

Using more types will also result in smaller serialized tagging data, and also less type conversion in the instrumentation code.

Could anyone share the rationale behind the current design?

sebright commented 6 years ago

/cc @dinooliva

ashi009 commented 6 years ago

It's also worth mentioning that, stackdriver supports more labeling data types than OC does. However, the existing OC library forces them to all use strings, even for http.status.

songy23 commented 6 years ago

In our initial design there are 3 types of TagValue: String, Boolean and Long (e.g https://github.com/census-instrumentation/opencensus-java/pull/564). IIUC later we found there are no clear use cases for Boolean and Long TagValues so we decided to start with String TagValue only.

/cc @rakyll @g-easy

ashi009 commented 6 years ago

I can argue that all of them have clear use cases. Strings can store all sorts of weirdness inside — even a base64 encoded proto message, but it’s hacky-ish. The biggest win to support more types is to simplify instrument code, as the user don’t have to cast them around and worrying about their string formatting — let the exporters to do the hard work if they don’t support the native typing system. Yang Song notifications@github.com于2018年7月10日周二上午12:47写道：

In our initial design there are 3 types of TagValue: String, Boolean and Long (e.g census-instrumentation/opencensus-java#564 https://github.com/census-instrumentation/opencensus-java/pull/564). IIUC later we found there are no clear use cases for Boolean and Long TagValues so we decided to start with String TagValue only.

/cc @rakyll https://github.com/rakyll @g-easy https://github.com/g-easy

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/census-instrumentation/opencensus-specs/issues/133#issuecomment-403544504, or mute the thread https://github.com/notifications/unsubscribe-auth/AAp9B8EVEeiren8NydeDkhoDyP-UbnMsks5uE4k5gaJpZM4VH9yr .

songy23 commented 6 years ago

Strings can store all sorts of weirdness inside — even a base64 encoded proto message, but it’s hacky-ish. The biggest win to support more types is to simplify instrument code, as the user don’t have to cast them around and worrying about their string formatting.

Is there a reason that you want to have long/boolean values as TagValue, instead of just a value for a Measurement? Or say, Is there a use case for propagating long/boolean TagValues on the wire? We do have a plan to add support for other types of Measurement, while TagValues are more for propagation purpose.

ashi009 commented 6 years ago

The current value types for measurements are sort of acceptable. But I’d be happy to see more types being added.

Some examples for tags (both for measurement and propagation, as a propagated tag will be consumed by measurement, tracing or logging):

a boolean tag for marking if a user is authenticated, which is better than tagging with PII
an int64 tag for enumerated value, eg. type of a request (eg. read write admin)
an int64 campaign id for tracking effectiveness of ads
a int64 timestamp for a rarely updated cache entry, use creation time as it’s version

Yang Song notifications@github.com于2018年7月10日周二上午1:24写道：

Strings can store all sorts of weirdness inside — even a base64 encoded proto message, but it’s

hacky-ish. The biggest win to support more types is to simplify instrument code, as the user don’t have to cast them around and worrying about their string formatting.

Is there a reason that you want to have long/boolean values as TagValue, instead of just a value for a Measurement? Or say, Is there a use case for propagating long/boolean TagValues on the wire? We do have a plan to add support for other types of Measurement, while TagValues are more for propagation purpose.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/census-instrumentation/opencensus-specs/issues/133#issuecomment-403555577, or mute the thread https://github.com/notifications/unsubscribe-auth/AAp9BwmT_6Quq-OKSuLhFCAgiH_ifCoyks5uE5G0gaJpZM4VH9yr .

rakyll commented 6 years ago

@ashi009

Stackdriver recommended us not to support multiple types because they are not practically useful and increasing the API surface. We removed supported for typed labels. OTOH, most backends today only support string values.

Given value is only useful when querying, a string value is sufficient. The example tags you have can be represented with a string value.

ashi009 commented 6 years ago

@rakyll Understood. Although non-string tags are not supported by many backends, it still should be the exporters' duty to convert them when necessary. By doing that, it simplifies code instrumentation, removes boilerplates also reduces the chance of making a mistake caused by inconsistent data conversions.

Take one example from my previous reply

a int64 timestamp for a rarely updated cache entry, use creation time as it’s version

Let's instrument a fake GRPC cache server as described above:

var (
    methodKey  = tag.New("cache.method")
    versionKey = tag.New("cache.version")
)

type server struct {
    version time.Time

    // stats
    invokeEvent     stats.Measurement
    invokeEventView view.View
}

func (s *server) Get(ctx contex.Context, req pb.GetRequest) (*pb.GetResponse, error) {
    ctx, _ = tag.New(ctx,
        tag.Insert(methodKey, pb.METHOD_GET.String()),
        tag.Insert(versionKey, strconv.FormatInt64(s.version.Unix(), 10)),
    )
    stats.Record(ctx, invoke.M(1))

}

func (s *server) Put(ctx contex.Context, req pb.PutRequest) (*pb.PutResponse, error) {
    ctx, _ = tag.New(ctx,
        tag.Insert(methodKey, pb.METHOD_PUT.String()),
        tag.Insert(versionKey, strconv.FormatInt64(s.version.Unix(), 10)),
    )
    stats.Record(ctx, invoke.M(1))

}

To make those tags into strings, I have to cast both the method and version around. As their string representation are derived from its original type, it may result in different tags for the same input if the conversion is not consistent all the time.

For instance, pb.METHOD_XXX can be cast with String(), which can also be cast with strconv.FormatInt32(). The version tag can also be an RFC3339 string for the same value, instead of a string of seconds from unix epoch in decimal.

Whoever instruments the code needs to know the conversion rule to avoid polluting the data. That's being said, the string tag is error prune by design.

The imaginary API interaction could be:

type Key interface { key() }

type Tag struct { key, value string }

// Int64 implements Key
func Int64(name) *Int64 {}
func(Int64) key() {}
func(Int64) WithValue(val int64) Tag;

// ctx, err := New(ctx, Insert(someKey.WithValue(someValue)), ...)

rakyll commented 6 years ago

Whoever instruments the code needs to know the conversion rule to avoid polluting the data.

Whoever instruments the code needs to know the conversion rule. The tags are designed by their users, we cannot be strongly opinionated here.

pb.METHOD_XXX casted to string or int32 is a user choice.

The imaginary API interaction could be:

I don't why we need to increase the core API surface. You can simply write these a function and use it everywhere if strconv.FormatInt64 is too boilerplate:

func intValue(v int64) string {
   return strconv.FormatInt64(v, 10))
}

tag.Insert(versionKey, intValue(s.version.Unix())),

ashi009 commented 6 years ago

Whoever instruments the code needs to know the conversion rule. The tags are designed by their users, we cannot be strongly opinionated here.

Agreed for the user tags. I'm more focused on the server tags, which often has a strong opinion on the format and consistency of the tags.

I don't why we need to increase the core API surface. You can simply write these a function and use it everywhere if strconv.FormatInt64 is too boilerplate:

I can see where this comes from, as I have considered and tried this solution already. It turned out to be problematic as there is no way to bind the conversion rule to a tag.Key. Every time someone touches those stuff, it's the reviewer's burden to make sure the rules are followed.

Another approach I can think of is to export an interface (like flag.Value) and make the Mutator c-tors to take that instead.

census-instrumentation / opencensus-specs

Question: Why tag value has to be string? #133