ECS Fargate/Dogstatsd task_arn tag on with orchestrator cardinality #5324
Conversation
Co-Authored-By: Ahmed Mezghani <38987709+ahmed-mez@users.noreply.github.com>
xornivore
changed the title
| lastExpire time.Time | ||
| expireFreq time.Duration | ||
| labelsAsTags map[string]string | ||
| doOnceOrchScope sync.Once |
There was a problem hiding this comment.
nit, why don't we embed the struct?
| doOnceOrchScope sync.Once | |
| sync.Once |
There was a problem hiding this comment.
I feel pretty strongly that we should encourage using the same sync.Once for anything else other than this orchestrator scope, if that changes we may want to revisit, But also consider that If we embed it, Do becomes an exported method for ECSFargateCollector while it's actually more of an implementation detail that we use sync.Once.
There was a problem hiding this comment.
Probably worth a comment for the scope of this once.
| @@ -501,6 +502,16 @@ func findOriginTags(origin string) []string { | |||
| tags = append(tags, originTags...) | |||
| } | |||
| } | |||
|
|
|||
| // Include orchestrator scope tags if the cardinality is set to orchestrator | |||
There was a problem hiding this comment.
Nit: should we really do that in findOriginTags? We're not testing for listeners.NoOrigin and it not seems that much related (I may be wrong on the second part). Feel free to ignore.
There was a problem hiding this comment.
So findOriginTags carries the notion of get tags for the case of "no entity id provided". Not sure if @ahmed-mez or @clamoriniere have an opinion on this?
There was a problem hiding this comment.
yes technically we're not adding origin tags here 🤔
| lastExpire time.Time | ||
| expireFreq time.Duration | ||
| labelsAsTags map[string]string | ||
| doOnceOrchScope sync.Once |
There was a problem hiding this comment.
Probably worth a comment for the scope of this once.
What does this PR do?
Enrich dogstatsd metrics with
task_arntag ifDD_DOGSTATSD_TAG_CARDINALITY=orchestrator.Motivation
Automatically inject
task_arntag to all the dogstatsd metric.Additional Notes
#3159