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

Incorrectly assuming that job label is always there when replacing %%juju_topology%% #406

Open
sed-i opened this issue May 28, 2024 · 4 comments

Comments

@sed-i
Copy link
Contributor

sed-i commented May 28, 2024

Bug Description

When using LogForwarder, it is incorrect to replace the topology stub with a job matcher.

Following up on #332, we can no longer assume that the job label would always be there, because pebble does not create it.

alert_rule["expr"] = self.tool.inject_label_matchers(
re.sub(r"%%juju_topology%%", r'job=~".+"', alert_rule["expr"]),
self.topology.label_matcher_dict,
)

Originally reported by @saltiyazan.

Related:

To Reproduce

NTA

Environment

NTA

Relevant log output

NTA

Additional context

No response

@saltiyazan
Copy link

Thanks for opening this @sed-i I wasn't sure if it was a bug or a misconfiguration on my side :)

@PietroPasotti
Copy link
Contributor

this happened because in the pebble log forwarder never added the 'job' and 'instance' labels. --> @barrettj12

hopefully it will be fixed and the fix backported to the pebbles in juju 3.4 and up

@sed-i
Copy link
Contributor Author

sed-i commented May 30, 2024

Just realized that our lib should probably be the one creating it, just like we do all other labels:

log_target.update(
{
"labels": {
"product": "Juju",
"charm": topology._charm_name,
"juju_model": topology._model,
"juju_model_uuid": topology._model_uuid,
"juju_application": topology._application,
"juju_unit": topology._unit,
},

@barrettj12
Copy link

Personally I supported adding job and instance labels by default to Pebble, but I know there was some opposition to this and we ultimately decided against it.

Is it possible for the charm just to manually set these labels in the Pebble plan?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants