librato / python-librato

A Python wrapper for the Librato Metrics API.
Other
72 stars 31 forks source link

Queue tags overwrite event tags #171

Closed mtanski closed 7 years ago

mtanski commented 7 years ago

In code at https://github.com/librato/python-librato/blob/master/librato/queue.py#L66 the queue tags overwrite tags included with the individual measurement.

        if len(self.tags) > 0:
            query_props['tags'] = self.tags

        if 'tags' in query_props:
            self.add_tagged(name, value, **query_props)

It seams like this should be additive.

        if len(self.tags) > 0:
            tags = {}
            self.tags.update(tags)
            query_props.get('tags', {}).update(tags)
            query_props['tags'] = tags

        if 'tags' in query_props:
            self.add_tagged(name, value, **query_props)

This leads to the following code to miss behave:

>>> api = librato.connect("test", "test", tags={"env": "env"})
>>> queue = api.new_queue()
>>> queue.add('test.mtanski', 1, type="counter", tags=Stat('test', 'test', 'test').__dict__)
>>> pdb.run('queue.submit()')
> <string>(1)<module>()
(Pdb) b librato/__init__.py:169
Breakpoint 3 at /Users/mtanski/world/env/2u7/lib/python2.7/site-packages/librato/__init__.py:169
(Pdb) c
> /Users/mtanski/world/env/2u7/lib/python2.7/site-packages/librato/__init__.py(169)_make_request()
-> conn.request(method, uri, body=body, headers=headers)
(Pdb) p body
'{"measurements": [{"count": 1, "sum": 1, "name": "test.mtanski", "tags": {"env": "env"}}], "tags": {"env": "env"}}'

This took me far to long to figure out. We had tags setup per client (and queue) and individual queue.add measurements could have their own tags.

mtanski commented 7 years ago

Confirmed with latest check out as well.

mbeale commented 7 years ago

@mtanski I have found the issue. The issue is the top level tags should be ignored when a measurement level tag is passed but top level tags are being used if specified. I have a solution I'm finalizing that will use the top level tags if no measurement level tags are specified.

Would that address your issue or would you expect measurement level tags to inherit from top level tags?

mtanski commented 7 years ago

@mbeale Instinctively I expected the tags at each level to be inherited by a lower level (or overwritten if specified). I can workaround it, if that's now how it works.

My use case is: As you can see above my example above has the global client creation where I specify the "environment" it's running it. I don't expect some low level code emitting events to know all the details of the environment (like prod vs. beta, or what app it's running in if it's part of the library).

mbeale commented 7 years ago

@mtanski I have a PR up which follows the tag inheritance that we have in other libraries.

https://github.com/librato/python-librato/pull/172

Le me know what you think.

mtanski commented 7 years ago

Seams like it'd do the trick. Wish tag inheritance was the default but I can live without it.

mtanski commented 7 years ago

As designed in PR #172 this works for me when using the extra inherit_tags kwarg.