Skip to content

fix: check for non-nil Locality before use in group_processes #50

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.

Sign up for GitHub

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

Merged
singler merged 1 commit into from
Apr 2, 2025
Merged

fix: check for non-nil Locality before use in group_processes #50

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion metrics/group_processes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ func (p *ProcessesMetricGroup) getTags(processName string, process *models.Proce
// For no Kubenetes deployments, metrics will be tagged with the machine hostname and the address of the process
tags["machine_id"] = process.MachineId
tags["address"] = process.Address
tags["zone"] = process.Locality.ZoneId
if process.Locality != nil {
tags["zone"] = process.Locality.ZoneId
}
Copy link
Copy Markdown

@MantasMiksys MantasMiksys Apr 2, 2025

Choose a reason for hiding this comment

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

Shall we maybe set "zone" tag to "unknown" in case Locality info is missing?

Copy link
Copy Markdown
Contributor Author

@singler singler Apr 2, 2025

Choose a reason for hiding this comment

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

I'm not sure what would be better in this case. In other codepath we're setting zone only if locality is available, so to have parity with that behavior just not setting it seems to be better

Copy link
Copy Markdown
Collaborator

@pboros pboros Apr 2, 2025

Choose a reason for hiding this comment

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

Yes, we should set to unknown, otherwise tally will crash if there are time series with different tags.

} else {
// Try to tag the name with the human readable name (as published by the operator) instead of the key in the json
if process.Locality != nil {
Expand Down