Parametrized/periodic jobs per child tagged metric emmision #4392

burdandrei · 2018-06-07T14:04:30Z

continuing better metrics emission started in #3882
fixes #4061

jippi · 2018-06-08T13:58:08Z

Given how high cardinality this telemetry will be, it would be nice to opt-in for it - influxdb will not like a lot of batch jobs emitting these extra labels often (like when jobs run every 5min 24/7)

Influx limit by default to just 1m series - which will run out very very quick already

burdandrei · 2018-06-08T14:10:28Z

@jippi, InfluxDB-wize there is no difference.
Current status is that you receive dispatch/periodic I'd as a part of job name.
My PR makes parent job name to be emitted as job name and dispatch/periodic I'd as a tag

burdandrei · 2018-06-12T08:54:30Z

ping @dadgar

preetapan · 2018-06-19T15:22:56Z

@burdandrei could you resolve the merge conflict on your branch with a rebase?

One concern I have is that if the change will break existing users that somehow relied on parsing the jobid to get the parent and the dispatched child id out. If we decide to merge this we need to mention that in the changelog and in the metrics documentation as well.

preetapan · 2018-06-19T21:03:12Z

client/task_runner.go

@@ -288,6 +284,31 @@ func NewTaskRunner(logger *log.Logger, config *config.Config,
 		},
 	}

+	if tc.alloc.Job.ParentID != "" {
+		tc.baseLabels = append(tc.baseLabels, metrics.Label{


Discussed this with the team internally, and we don't want to break any existing users that rely on the full jobID for dispatched jobs. Could you add a new "parentJob" label here and below for alloc.job.ParentID and leave the existing "job" label as it is? That way it doesn't break backwards compatibility.

burdandrei · 2018-06-20T11:39:35Z

@preetapan
rebase - done
task_runner - reverted job tag change, added parent_id + (dispatch/periodic_id)

What about summaries? Does it look ok?

preetapan · 2018-06-20T20:57:10Z

nomad/leader.go

+						}
+						labels = append(labels, metrics.Label{
+							Name:  "job",
+							Value: metricJobId,


This is still not quite right. The "job" label should have summary.JobId in all three cases, so I would suggest adding back the code in lines 619-622, and removing 646-648. In each of your case statements above for dispatch and periodic, jobInfo[0] should be added as new label called "parentJobId".

For example, if summary.JobID is 'myjob/dispatch-23423423', this should result in the following labels:

label value

"job" myjob/dispatch-23423423 --> (this maintains backwards compatibility)

"parentJob" myjob

"dispatch_id" 23423423

Got you,
generally, I tried to avoid this, cause if I got a dashboard with dropdown by JOB ID and lot's of periodic/dispatch it's really hard to find normal jobs. But nomad-wise you are right job id is the long one. Maybe it's better to add an option to config emit/skip for parametrized/periodic jobs. But again it can add redundant operational complexity.
Let me know if you want me to provide the config option.

burdandrei · 2018-06-21T15:37:44Z

thanks, @preetapan!
can we add #3882 with this as well?

chelseakomlo · 2018-06-21T16:50:59Z

nomad/leader.go

+								Value: jobInfo[0],
+							}, metrics.Label{
+								Name:  "periodic_id",
+								Value: jobInfo[1],


Ensure that the length of jobInfo is what is expected for safety.

In this specific case, the contains check in line 642 guards against that. If the string ends with /periodic- that slice should still have 2 elements with one being the empty string.

chelseakomlo · 2018-06-21T16:53:24Z

client/allocrunner/taskrunner/task_runner.go

+		if strings.Contains(tc.alloc.Job.Name, "/periodic-") {
+			tc.baseLabels = append(tc.baseLabels, metrics.Label{
+				Name:  "periodic_id",
+				Value: strings.Split(tc.alloc.Job.Name, "/periodic-")[1],


First check the length after splitting the string to ensure the length is as expected before assigning the second element.

chelseakomlo · 2018-06-21T16:53:49Z

client/allocrunner/taskrunner/task_runner.go

+		if strings.Contains(tc.alloc.Job.Name, "/dispatch-") {
+			tc.baseLabels = append(tc.baseLabels, metrics.Label{
+				Name:  "dispatch_id",
+				Value: strings.Split(tc.alloc.Job.Name, "/dispatch-")[1],


See below comment. Consider pulling this into a helper function to reduce code duplication.

thanks for the input, I wrote helper function hour ago and now I'm trying to figure where to place it =)

In this case, its probably fine to not introduce a helper method. This and the other place we do this, are in two different packages and we would have to put one helper method in a util package that both can import. Doesn't seem worth it this time..

That what's stopped me - 2 different packages without imports intersection.
Thanks @preetapan, and thanks @chelseakomlo for really usefull code review tips!

chelseakomlo · 2018-06-21T16:55:40Z

nomad/leader.go

+								Value: jobInfo[0],
+							}, metrics.Label{
+								Name:  "dispatch_id",
+								Value: jobInfo[1],


Don't assume that this array will have two elements, first check for safety.

chelseakomlo · 2018-06-21T16:57:25Z

nomad/leader.go

+							}, metrics.Label{
+								Name:  "periodic_id",
+								Value: jobInfo[1],
+							})


Two points overall:

Consider adding a helper function where there is code duplication

Don't assume the length of the array is what is as expected, first check before indexing.

PR #4392 was merged to master *after* allocrunnerv2 was branched, so the client-specific portions must be ported from master to arv2.

github-actions · 2023-03-02T02:19:38Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

burdandrei force-pushed the telemetry-parametrized-jobs branch 2 times, most recently from 3159c4e to a0a9f1f Compare June 7, 2018 18:36

burdandrei force-pushed the telemetry-parametrized-jobs branch from a0a9f1f to ff1b0d4 Compare June 10, 2018 11:57

preetapan reviewed Jun 19, 2018

View reviewed changes

burdandrei force-pushed the telemetry-parametrized-jobs branch from ff1b0d4 to 05d5523 Compare June 20, 2018 11:35

preetapan reviewed Jun 20, 2018

View reviewed changes

Parametrized/periodic jobs per child tagged metric emmision

4e352f3

burdandrei force-pushed the telemetry-parametrized-jobs branch from 05d5523 to 4e352f3 Compare June 21, 2018 07:41

preetapan approved these changes Jun 21, 2018

View reviewed changes

preetapan mentioned this pull request Jun 21, 2018

Improve telemetry docs #4440

Merged

chelseakomlo suggested changes Jun 21, 2018

View reviewed changes

preetapan merged commit 7325cf6 into hashicorp:master Jun 21, 2018

schmichael added a commit that referenced this pull request Oct 10, 2018

Port client portion of #4392 to new taskrunner

d61735a

PR #4392 was merged to master *after* allocrunnerv2 was branched, so the client-specific portions must be ported from master to arv2.

schmichael mentioned this pull request Oct 10, 2018

Port client portion of #4392 to new taskrunner #4767

Merged

schmichael added a commit that referenced this pull request Oct 12, 2018

Port client portion of #4392 to new taskrunner

f2d801f

PR #4392 was merged to master *after* allocrunnerv2 was branched, so the client-specific portions must be ported from master to arv2.

schmichael added a commit that referenced this pull request Oct 15, 2018

Port client portion of #4392 to new taskrunner

d63300d

PR #4392 was merged to master *after* allocrunnerv2 was branched, so the client-specific portions must be ported from master to arv2.

schmichael added a commit that referenced this pull request Oct 16, 2018

Port client portion of #4392 to new taskrunner

34d35b8

PR #4392 was merged to master *after* allocrunnerv2 was branched, so the client-specific portions must be ported from master to arv2.

schmichael added a commit that referenced this pull request Oct 16, 2018

Port client portion of #4392 to new taskrunner

2256917

PR #4392 was merged to master *after* allocrunnerv2 was branched, so the client-specific portions must be ported from master to arv2.

burdandrei mentioned this pull request Oct 18, 2018

job name tagging in telemetry should be less specific for periodic jobs (should not include /periodic-1235... info) #4646

Closed

github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parametrized/periodic jobs per child tagged metric emmision #4392

Parametrized/periodic jobs per child tagged metric emmision #4392

burdandrei commented Jun 7, 2018

jippi commented Jun 8, 2018

burdandrei commented Jun 8, 2018

burdandrei commented Jun 12, 2018

preetapan commented Jun 19, 2018

preetapan Jun 19, 2018

burdandrei commented Jun 20, 2018

preetapan Jun 20, 2018

burdandrei Jun 21, 2018

burdandrei commented Jun 21, 2018

chelseakomlo Jun 21, 2018

preetapan Jun 21, 2018

chelseakomlo Jun 21, 2018

chelseakomlo Jun 21, 2018

burdandrei Jun 21, 2018

preetapan Jun 21, 2018

burdandrei Jun 22, 2018

chelseakomlo Jun 21, 2018

chelseakomlo Jun 21, 2018

github-actions bot commented Mar 2, 2023

label	value
"job"	myjob/dispatch-23423423 --> (this maintains backwards compatibility)
"parentJob"	myjob
"dispatch_id"	23423423

Parametrized/periodic jobs per child tagged metric emmision #4392

Parametrized/periodic jobs per child tagged metric emmision #4392

Conversation

burdandrei commented Jun 7, 2018

jippi commented Jun 8, 2018

burdandrei commented Jun 8, 2018

burdandrei commented Jun 12, 2018

preetapan commented Jun 19, 2018

Choose a reason for hiding this comment

burdandrei commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burdandrei commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 2, 2023