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

[APM Tracing: Go] Update DD_TAGS documentation #27110

Closed
wants to merge 1 commit into from

Conversation

mtoffl01
Copy link
Contributor

What does this PR do? What is the motivation?

Updates documentation on DD_TAGS. Default value of [] was incorrect because env var values are in string format. By default, no global tags are collected. Also updated the description to specify that key-value pairs must be string-convertible types.
Motivation: Customer request

Merge instructions

Merge readiness:

  • Ready for merge

Merge queue is enabled in this repo. To have it automatically merged after it receives the required reviews, create the PR (from a branch that follows the <yourname>/description naming convention) and then add the following PR comment:

/merge

Additional notes

@mtoffl01 mtoffl01 requested a review from a team as a code owner January 14, 2025 18:27
@mtoffl01 mtoffl01 requested a review from darccio January 14, 2025 18:27
@KaibutsuX
Copy link

Assuming the file in this pr only applies to go code (go.md), then the description is still incorrect.
According to the parsing code:

func statsTags(c *config) []string {                                                                                              
    tags := globalconfig.StatsTags()                                                                                              
    if c.serviceName != "" {                                                                                                      
        tags = append(tags, "service:"+c.serviceName)                                                                             
    }                                                                                                                             
    // TODO: grab tracer config's env and hostname for globaltags                                                                 
    for k, v := range c.tags {                                                                                                    
        if vstr, ok := v.(string); ok {                                                                                           
            tags = append(tags, k+":"+vstr)                                                                                       
        }                                                                                                                         
    }                                                                                                                             
    return tags                                                                                                                   
}     

The doc states:

Tags can be separated by commas or spaces, for example: `layer:api,team:intake,key:value` or `layer:api team:intake key:value`

Tags may not be in that format because they are parsed individually as a string:interface{} value. Multiple tags would not be supported.

Copy link
Contributor

@maycmlee maycmlee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mtoffl01 for opening this PR! It looks good to me, but could you open a PR with the branch named in the format: your-name/branch-name?

@mtoffl01 mtoffl01 closed this Jan 15, 2025
@mtoffl01 mtoffl01 deleted the mtoffl01-patch-1 branch January 15, 2025 15:11
@mtoffl01
Copy link
Contributor Author

Hi @KaibutsuX ,

This branch is closed in favor of another branch, but the information in the new PR will be the same.

Two sets of tags are referenced in the code snippet you provided: "global tags" and "local tags."

DD_TAGS, or "global tags," accepts input like this: DD_TAGS="key1:value2,key2:2,key3:true" where comma and space separators are equally supported. Global tags are stored in a map[string]interface{} internally, but converted to a []string that holds key-value pairs like so: {"key1:value1","key2:value2"} and is put onto our globalconfig (Reference). This array is later retrieved by contrib/database/sql (Reference), and merged with "local tags" set on contrib/database/sql (see c.tags), where "local tags" have been set via the WithCustomTags option. The union of both sets of tags is used to populate the tags on our statsd client.

I'm not clear on what you mean here,

Tags may not be in that format because they are parsed individually as a string:interface{} value. Multiple tags would not be supported.

But should you have any issue with DD_TAGS, let's re-route that discussion to the existing dd-trace-go PR.

Thanks!

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.

4 participants