Added Additional Metrics and Job ID in the Docker Source #4547
Conversation
| Help: "Total number of Docker layers scanned.", | ||
| }, | ||
| []string{"source_name"}) | ||
| []string{"source_name", "job_id"}) |
There was a problem hiding this comment.
Why did you add job_id for all of these? I know that we use it as a metric label in many other places, but that kind of contravenes best Prometheus practices, so I'd like to be very deliberate about extending the pattern.
There was a problem hiding this comment.
Hey @rosecodym! Thanks for reviewing this and calling that out.
The original requirement I was given was “add the source’s Job ID to all metrics calls” so I pushed job_id onto the shared label set for all Docker metrics, the reason I believe is so that metrics from different scan jobs can be distinguished instead of being aggregated into the same series.
Though, hmm... thinking about it from a Prometheus perspective I do agree we probably don’t need job_id on every one of these series, especially the histogram.
I can perhaps scope this down so that job_id is only applied to the progress counters/gauges and leave docker_list_images_api_duration_seconds with just the source_name?
I would like to know if that sound reasonable to you, or do you have any other suggestion? Let me know if I have completely misunderstood your comment 😅.
There was a problem hiding this comment.
Why did you add job_id for all of these?
This was my idea, I wanted to have a way of noticing big changes in these numbers between jobs. But, I'm a Prometheus dummy so if this is a bad idea or there's better ways to do it, I'm into it. I think there's one metric we don't need it for though; I'll comment there.
pkg/sources/docker/metrics.go
Outdated
| Buckets: prometheus.DefBuckets, | ||
| }, | ||
| []string{"source_name"}) | ||
| []string{"source_name", "job_id"}) |
There was a problem hiding this comment.
This is the one I don't think we need job_id for
There was a problem hiding this comment.
Ah missed this, yes this is what I had in mind as well, wouldn't need this on the histogram
camgunz
left a comment
camgunz
left a comment
There was a problem hiding this comment.
Let's remove job_id from "docker_list_images_api_duration_seconds", then I think we're good to go.
| // Chunks emits data over a channel that is decoded and scanned for secrets. | ||
| func (s *Source) Chunks(ctx context.Context, chunksChan chan *sources.Chunk, _ ...sources.ChunkingTarget) error { | ||
| ctx = context.WithValues(ctx, "source_type", s.Type(), "source_name", s.name) | ||
| jobIDStr := fmt.Sprint(s.jobId) |
There was a problem hiding this comment.
Wow also TIL I learned about fmt.Sprint; verrrrry handy
nabeelalam
deleted the
update/source/add-new-metrics-and-job-id-in-docker
branch
Description:
This PR adds three new Docker source metrics for improved visibility into image enumeration and scanning progress.
It also updates all Docker metrics to include the "Job ID" label alongside "Source Name" for better observability of the statistics.
New Metrics:
Checklist:
make test-community)?make lintthis requires golangci-lint)?