Skip to content
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

Introduce a common abstractions to handle tags #572

Merged
merged 4 commits into from
Apr 1, 2019

Conversation

ivantopo
Copy link
Contributor

@ivantopo ivantopo commented Mar 11, 2019

⚠️ Warning: This PR builds on top of #569 and should not be merged until that PR is merged.

Background and Motivation

We are heavy users of the "tags" term around Kamon, we currently have metric tags, span tags and since #552 we also have context tags (merged but not released yet). Still, tags were not treated equally all around. In the metrics and context sections they were a Map[String, String] but on the tracer tags could be Boolean, Long or String values. Furthermore, accessing the tags might be annoying, specially if accessing them from non-Scala language (Java, I'm looking at your) so the idea came to mind: what if we could have a single abstraction for handling a group of key/value pairs and use it throughout the entire code base? so the work on this PR started.

Goals

This is how we hope this PR will improve Kamon's user experience:

  • Have a single abstraction that can be used to handle tags in metrics, spans, context and future ideas like events.
  • Make sure that the user experience around tags is decent from both Java and Scala, specially given the fact that with context tags it will be a lot easier for users to start using tags and maybe wanting to copy them from the context into metrics or spans.

Solution on this PR

We are introducing two main concepts on this PR:

  • A TagSet which is an immutable set of key/value pairs meant to replace the previous Map[String, String] and similar implementations. TagSet instances can be constructed from existing maps or by explicitly providing each key/value pair to be included in it.
  • A TagSet.Lookup interface and several built-in implementations of it. A Lookup'a task is to take a raw, untyped map that is used as the actual storage for the tags and return certain representation of the value. It can be that some users want to get a String or null if not present, or an Optional[String], or just force whatever is there on the map to become a string (specially useful when logging things from the Context). Lookups provide a simple and typed way of accessing the contents of a TagSet, without prescribing or limiting the ways in which missing or unexpected values are treated.

Here is an example of how using these tags look like:

val tags = TagSet.from(Map(
  "age" -> 5L,
  "name" -> "Kamon",
  "isAwesome" -> true
)

// returns a plain string or null if the "name" tag doesn't exist or isn't a string.
tags.get(plain("name")) 

// returns the value of the "name" tag as an Optional[String], or empty if missing
// or not a string.
tags.get(optional("name"))

// returns an Option[Boolean]. Notice that type prefix on the lookup.
tags.get(booleanOption("isAwesome"))

// returns a String with the value of the "age" tag, regardless of its type.
tags.get(coerce("age"))

Lookup names are biased towards String values since they tend to be by far the most common case, that's what you can get a plain String but you need to use plainLong or plainBoolean if you are searching for a values with long or boolean types, respectively.

Finally, notice the coerce Lookup which will always return a String value, regardless of the actual type of the tag, which should come in very handy when trying to log tags from a Context.

Scope

As part of this PR I'm only changing the implementation of Tags for context Tags and including all the required changes for this to work when doing Context propagation over HTTP and Binary transports and separate PRs will be issued for introducing these changes to the Metrics and Tracing APIs as well.

@ivantopo ivantopo mentioned this pull request Mar 14, 2019
@ivantopo ivantopo marked this pull request as ready for review March 18, 2019 12:45
@ivantopo
Copy link
Contributor Author

I have been doing a few tests over this PR, specially around how much time it takes to actually create and modify TagSet instances, taking into account that the most common case we see is for metrics and Spans to have between 4 and 6 tags and concluded that two important changes were needed:

  1. Using something other than the Scala immutable map as the underlying storage because it behaves nicely up until 4 tags, but then copying a TagSet can become quite expensive. After doing some benchmarks using the scala Map, TreeMap, HashMap and finally the EclipseCollections UnifiedMap it seems like the one offering the best balanced behavior is the UnifiedMap so I changed the underlying storage to use it. The entire results from the benchmarks are here: https://gist.github.com/ivantopo/33643677deb9f7004fa47927fa0a8e19 and here is a quick summary of those results:

image
One lesson of the story would be: if you know that you are creating a TagSet from several tags, use a TagSet.Builder to add them all and then create the TagSet from it.

  1. Lookups should not be directly exposed to the underlying storage but instead to the new TagSet.Storage interface. This ensures that we can change the underlying storage implementation without affecting any external code.

The only thing left to figure out later is reducing the packaged size of core. The Eclipse Collections dependency is being repackaged as we do with some other dependencies, but it adds like 10+ MB of unnecessary classes to the packaged jar. I'll get into removing everything that is not necessary from there when we get closer to the release.

@dpsoft
Copy link
Contributor

dpsoft commented Mar 28, 2019

@ivantopo Awesome!! The only thing I have to say is it adds like 10+ MB of unnecessary classes to the packaged jar, but that you already knew ;)

@ivantopo
Copy link
Contributor Author

Yeah 😢... I was wondering whether to copy the specific parts of UnifiedMap that we use into Kamon or get into proguarding stuff, not liking any of those options but given that the results are awesome we will have to do something!

@ivantopo ivantopo merged commit 45237e2 into kamon-io:master Apr 1, 2019
@ivantopo ivantopo mentioned this pull request Apr 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants