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

Feature request (again): Tags #53

Closed
bconway opened this issue Dec 12, 2020 · 8 comments
Closed

Feature request (again): Tags #53

bconway opened this issue Dec 12, 2020 · 8 comments

Comments

@bconway
Copy link

bconway commented Dec 12, 2020

Since the last issue over two years ago (#38), Graphite, Datadog, and InfluxDB have all accepted tags as relatively important functionality, with slightly different formatting between them (example: https://github.com/smira/go-statsd/pull/29/files). Would it be possible to support this?

@dropwhile
Copy link
Member

@bconway As mentioned in the previous issue (#38), the current send pipeline was not really designed with tag support in mind, and as such there are some send pipeline and buffer optimizations that make it a bit harder to just glue on tag support.

However, I'm willing to take another look when I get some available time, and see how difficult it would be to add tag support (without breaking the existing api for existing users). I've been busy with work lately though, so it might take a while to dig into things.

@dropwhile
Copy link
Member

dropwhile commented Jan 3, 2021

@bconway I've come up with two possible ways to accomplish this in some local test code.
Let me know which method you consider more of an appealing api.

proposal 1: Subclient-style

This creates a subclient that "owns" the tags. Perhaps helpful if you want to pass a tagged subclient off to a module that will send specific tags for every stat call.

config := &statsd.ClientConfig{
    Address: "127.0.0.1:8125",
    Prefix: "test-client",
}

// Now create the client
client, err := statsd.NewClientWithConfig(config)

// and handle any initialization errors
if err != nil {
    log.Fatal(err)
}

// make sure to close to clean up when done, to avoid leaks.
defer client.Close()

// Send a stat
client.Inc("stat1", 42, 1.0)

// Send a tagged stat (type: inline)
client.Tag([]Tag{{"tag1", "value1"}, {"tag2", "value2"}).Inc("stat1", 42, 1.0)

// Alternatively, create a tagged subclient, and use that to send many stats with
// the same tags
tags = []Tag{
    {"tag1", "value1"},
    {"tag2", "value2"},
)
tag_client := client.Tag(tags)

Pros

  • Api looks nice: client.Tag(...).Inc(..) is visually appealing, and succinct.
  • Allows a client with tags to be retained and called many times with the same set of tags across many metrics.
  • Less code duplication

Cons

  • Generates a bit of garbage with each client.Tag() call as a new sub-client is created. This client can be retained and called many times, but most users will likely go with the more succinct and clear call, and thus create a bit more garbage to collect.
  • Adds a tiny bit of slowdown to non tagged calls, as the submit path needs to check for the existence of tags as part of the formatting and processing step.

proposal 2: Duplicate methods, but with Tags

This just adds a duplicate set of methods to the client struct that accepts a tag list

config := &statsd.ClientConfig{
    Address: "127.0.0.1:8125",
    Prefix: "test-client",
}

// Now create the client
client, err := statsd.NewClientWithConfig(config)

// and handle any initialization errors
if err != nil {
    log.Fatal(err)
}

// make sure to close to clean up when done, to avoid leaks.
defer client.Close()

// Send a stat
client.Inc("stat1", 42, 1.0)

// Send a tagged stat (type: inline)
client.TagInc("stat1", 42, 1.0, []Tag{{"tag1", "value1"},{"tag2", "value2"}))

// Alternatively, create tags separately
tags = []Tag{
    {"tag1", "value1"},
    {"tag2", "value2"},
)
client.TagInc("stat1", 42, 1.0, tags)

Pros

  • Api is straightforward: client.TagInc(.., []Tag)
  • No additional garbage, aside from the list of tags ([]Tag) itself.
  • Less overhead to non-tag metric methods vs proposal 1. (Note: the difference is pretty small though)

Cons

  • Api does not look as nice as proposal 1.
  • Bit more code duplication than proposal 1.
  • Bloats the api footprint quite a bit (almost 2x) -- duplicates metric methods with a Tag prefix.

I'm leaning towards the second proposal, even though it results in more code and a bit of duplication, due to the reduction in memory garbage that the subclient generates when it is created and then just thrown away (when used in the client.Tag(...).* form. But most people probably won't be using the tag features anyway, so either is likely ok.

@bconway
Copy link
Author

bconway commented Jan 4, 2021

Thank you for that write-up, it was extensive and you've obviously put a lot of thought into it. I agree with all your pros and cons. The .Tag()... chain approach is popular, and the straightforward API of proposal 2 also offers a lot. To be honest, both look fine to me.

Have you considered a third approach, a variadic final arg?

func (s *Client) Inc(stat string, value int64, rate float32, tags ...Tag) error

I can't speak the garbage generation, but the API would remain backward compatible, I believe.

@dropwhile
Copy link
Member

dropwhile commented Jan 4, 2021

I did consider the variadic format, but discounted it because at one point long ago I remembered reading something about variadic functions in Go doing some extra heap allocation because the escape analytics couldn't ensure it wasn't used outside the function call... but thinking about it now that was a real long time ago, and is probably a non-issue now (I did some searching and only ran across very old posts about it).

...As such that does seem like another viable option too, and in fact seems preferable over the others I proposed. Nice!

I'll give that one a try, and see if it adds much overhead for non-tag calls.

dropwhile added a commit that referenced this issue Jan 5, 2021
*   Add Tag support: suffix-octothorpe, infix-comma, infix-semicolon (GH-53)
*   Remove previously deprecated NoopClient. Use a nil `*Client` Statter as a
    replacement, if needed. Ex:
    ```
    var client Client
    // A nil *Client has noop behavior, so this is safe.
    // It will become a small overhead (just a couple function calls) noop.
    err = client.Inc("stat1", 42, 1.0)
    ```
dropwhile added a commit that referenced this issue Jan 5, 2021
*   Add Tag support: suffix-octothorpe, infix-comma, infix-semicolon (GH-53)
*   Remove previously deprecated NoopClient. Use a nil `*Client` Statter as a
    replacement, if needed. Ex:
    ```
    var client Client
    // A nil *Client has noop behavior, so this is safe.
    // It will become a small overhead (just a couple function calls) noop.
    err = client.Inc("stat1", 42, 1.0)
    ```
*   bump major ver
@dropwhile
Copy link
Member

dropwhile commented Jan 5, 2021

work in progress branch here: https://github.com/cactus/go-statsd-client/tree/taggle-rock-2

usage:

// First create a client config. Here is a simple config that sends one
// stat per packet (for compatibility).
config := &statsd.ClientConfig{
    Address: "127.0.0.1:8125",
    Prefix: "test-client",
}

/*
// This one is an example of configuring "Tag" support
// Supported formats are:
//   InfixComma
//   InfixSemicolon
//   SuffixOctothorpe
// The default, if not otherwise specified, is SuffixOctothorpe.
config := &statsd.ClientConfig{
    Address: "127.0.0.1:8125",
    Prefix: "test-client",
    ResInterval: 30 * time.Second,
    TagFormat: statsd.InfixSemicolon,
}
*/

// Now create the client
client, err := statsd.NewClientWithConfig(config)

// and handle any initialization errors
if err != nil {
    log.Fatal(err)
}

// make sure to close to clean up when done, to avoid leaks.
defer client.Close()

// Send a stat
client.Inc("stat1", 42, 1.0)

// Send a stat with "Tags"
client.Inc("stat2", 41, 1.0, Tag{"mytag", "tagval"})

dropwhile added a commit that referenced this issue Jan 5, 2021
*   Add Tag support: suffix-octothorpe, infix-comma, infix-semicolon (GH-53)
*   Remove previously deprecated NoopClient. Use a nil `*Client` Statter as a
    replacement, if needed. Ex:
    ```
    var client Client
    // A nil *Client has noop behavior, so this is safe.
    // It will become a small overhead (just a couple function calls) noop.
    err = client.Inc("stat1", 42, 1.0)
    ```
*   bump major ver
@dropwhile
Copy link
Member

dropwhile commented Jan 11, 2021

@bconway this has been merged, but it is tagged as v5.0.0-alpha.0. To experiment with it, make the following modifications to your project's....

go.mod

require (
	github.com/cactus/go-statsd-client/v5 v5.0.0-alpha.0
)

using in code

import (
	"github.com/cactus/go-statsd-client/v5/statsd"
)

Once it gets some more testing, and barring any major issues, I'll tag it as v5.0.0

@dropwhile
Copy link
Member

closing as completed.

@bconway
Copy link
Author

bconway commented Jan 14, 2021

Much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants