feat: support space separated DD_TAGS value

[majorgreys]

Description

Closes #2195

Checklist

• Added to the correct milestone.
• Tests provided or description of manual testing performed is included in the code or PR.
• Library documentation is updated.
• Corp site documentation is updated (link to the PR).

[brettlangdon]

Should this have more helpful error message?

Should this raise? We only parse during startup right?

[majorgreys]

A more descriptive error message could be good but this hard to guess at when we don't know for sure which separator the user is intending. I could change it to: "Failed to find separator for tag string: '%s'.".

Being that it is internal, I think we can continue as is and allow it to catch the errors and log appropriately. If we are to raise, I think we should take that up in a follow-up since that'll require a bit of work elsewhere.

[brettlangdon]

By more descriptive, we could also do something like:

Failed to find separate for tag string: %r.
Tag string must be comma or space separated
one:value,two:value,three:value
one:value two:value three:value

Saying comma or space separated with an example of how it is suppose to look (even if we don't use their string/values as an example) could be helpful, otherwise that error message means you have to go look up in the docs (if we wanted to add an "errors" section to the docs and have a link to what this error message means in long form... that could be fine too... ? maybe?)

[P403n1x87]

WDYT of

    n = tags.count(":")
    for sep in ", ":
        if n == sum(":" in _ for _ in tags.split(sep)):
            break
    else:
        log.error(...)
        return parsed_tags

[P403n1x87]

In fact, I'm not sure this is even correct. Perhaps the better thing to do is

    for sep in ", ":
        if all(_.count(":") == 1 for _ in tags.split(sep)):
            break
    else:
        log.error(...)
        return parsed_tags

[majorgreys]

The only (minor) issue with the all(...) solution is that it would consider key1,key2,key3 an error having to do with determining the separator when it's that the tags themselves are not well-formed.

[P403n1x87]

Ok, forget what I said, the original approach is indeed fine! Sorry for all the confusion! 😞

[brettlangdon]

From looking at internal usage, I am still team raise an error:

$ git grep parse_tags_str
ddtrace/bootstrap/sitecustomize.py:25:from ddtrace.utils.formats import parse_tags_str
ddtrace/bootstrap/sitecustomize.py:63:    modules = parse_tags_str(modules_to_patch)
ddtrace/bootstrap/sitecustomize.py:113:        tracer.set_tags(parse_tags_str(env_tags))
ddtrace/profiling/exporter/http.py:18:from ddtrace.utils.formats import parse_tags_str
ddtrace/profiling/exporter/http.py:65:                parse_tags_str(os.environ.get("DD_TAGS")).items(),
ddtrace/profiling/exporter/http.py:66:                parse_tags_str(os.environ.get("DD_PROFILING_TAGS")).items(),
ddtrace/settings/config.py:9:from ..utils.formats import parse_tags_str
ddtrace/settings/config.py:56:        self.tags = parse_tags_str(os.getenv("DD_TAGS") or "")
ddtrace/settings/config.py:65:        self.service_mapping = parse_tags_str(get_env("service", "mapping", default=""))

But we can handle as a follow-up.

[P403n1x87]

This is so totally my fault, you had the logic right before my last rushed comments! I'm making this final suggestion, but feel free to ignore.

Suggested change

	if sum(1 for _ in tags_str.split(sep) if _.count(":") == 1) == numtagseps:
	if sum(":" in _ for _ in tags_str.split(sep)) == numtagseps:

[brettlangdon]

what about "key:value" ?

This regex will cause that use case to not work.

[majorgreys]

The regex is for two colons without a valid separator in between. There is a test case for the simple case you gave.

[brettlangdon]

oh wait... I think I completely misread this... this is looking to see if there is more than 1 tag without a space or comma?

[majorgreys]

yep, like "key2:val1:" test

[brettlangdon]

why do we need this check? does it help short circuit our parsing algorithm so we don't waste time parsing something that shouldn't, or does it catch an edge case that our parsing algorithm doesn't catch?

[majorgreys]

it's an edge case that would otherwise be treated as a problem with separators, which it might not be the case, e.g., key:val key2:val_with_a_colon_:.

[Yun-Kim]

Just one question but otherwise lgtm

[Yun-Kim]

Is this else block indented properly? What condition is this opposing?

[P403n1x87]

The else is there to catch the case that no valid separator has been identified.

[Yun-Kim]

Ahh, I wasn't familiar with the for | else clause until now, thanks for clarifying 😄

[Kyle-Verhoog]

Worth having a comment for this IMO for those who can't read regex (me)

[brettlangdon]

You might want to provide an example or two of what we are trying to test for.

[majorgreys]

This is documented already in the parse_tags_str whereas this comment is meant to be a gloss on the regex to make it easy to read.

[Kyle-Verhoog]

Suggested change

	Support space-separated `DD_TAGS`
	Add support for space-separated `DD_TAGS`.

[codecov-io]

Codecov Report

Merging #2196 (1d49dda) into master (2c4e4ce) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2196      +/-   ##
==========================================
- Coverage   91.60%   91.59%   -0.01%     
==========================================
  Files         569      569              
  Lines       40803    40791      -12     
==========================================
- Hits        37378    37364      -14     
- Misses       3425     3427       +2     

Impacted Files	Coverage Δ
ddtrace/utils/formats.py	96.29% <100.00%> (+0.94%)	⬆️
tests/tracer/test_utils.py	100.00% <100.00%> (ø)
ddtrace/contrib/sanic/patch.py	95.23% <0.00%> (-1.20%)	⬇️
ddtrace/profiling/exporter/http.py	98.91% <0.00%> (-1.09%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c4e4ce...1d49dda. Read the comment docs.