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

Fix position files #116

Merged
merged 3 commits into from
Mar 9, 2023
Merged

Fix position files #116

merged 3 commits into from
Mar 9, 2023

Conversation

dstathis
Copy link
Contributor

@dstathis dstathis commented Mar 8, 2023

Issue

Fixes #109

Solution

Use different names for the positions files

Testing Instructions

Relate over loki_push_api and check systemctl status snap.grafana-agent.grafana-agent.service

Release Notes

Fixed an issue where Grafana Agent could not start if related to loki.

@dstathis dstathis changed the base branch from main to machine_charm March 8, 2023 13:58
Copy link
Contributor

@rbarry82 rbarry82 left a comment

Choose a reason for hiding this comment

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

LGTM.

Amazing that every different scraper needs its own YAML instead of just making a new key.

Obviously the linter, again...

Copy link
Contributor

@Abuelodelanada Abuelodelanada left a comment

Choose a reason for hiding this comment

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

Shouldn't we put these in constants, so it is easy to modify in the future?

@rbarry82
Copy link
Contributor

rbarry82 commented Mar 8, 2023

For me, that's six of one and half a dozen of another. The only place the constant would be used is here, and it's swapping one hardcoded string for another.

If Grafana Agent isn't able to share, I wonder instead if we should make the name more dynamic, so it's something like:

name = # generate_some_meaningful_identifier
{
  "name": name,
  "positions": { "filename": f"/run/{name}_positions.yaml" },
  ..
}

To future proof a little against the possibility of some other possible need for a position file coming up in the future and us forgetting that bad things happen if they share.

@awnns
Copy link
Contributor

awnns commented Mar 8, 2023

I played around a little with this. You can also set a folder in which the grafana-agent will generate its own position files based on the job name so you don't have to set the name manually for every job. There's a field on the same level as "configs" called "positions_directory" (see https://grafana.com/docs/agent/latest/configuration/logs-config/). I had to put it in /var/snap/[...] to get it to work permissions wise, but that did the trick for me at least. Have you considered it?

@rbarry82
Copy link
Contributor

rbarry82 commented Mar 8, 2023

I don't know if any of us came across that in the docs, but it's a great find we should definitely use. Thanks @awnns !

@dstathis dstathis merged commit 0a2f496 into machine_charm Mar 9, 2023
@dstathis dstathis deleted the fix_position_files branch March 9, 2023 15:25
sed-i added a commit that referenced this pull request Apr 14, 2023
* Cleanup etc. (#90)
* Fix metadata and snap usage (#91)
* Class split (#92)
* Make static_configs into a list (#96)
* add option for tls insecure skip verify (#93)
* Add dashboards handler (#97)
* Scenario tests and machine charm rename (#99)
* spelling errors (#100)
* Clear machine metadata relations (#98)
* Add tests for update-status (#101)
* Type fixes (#102)
* small fixes and scenarios (#103)
* Machine charm lib (#95)
* Feature/fix prop (#111)
* LINT. Fix the grafana data iteration (#112)
* plug extraction (#105)
* Sub relation priority (#114)
* Fix a possible JSON loading exception (#119)
* purged grafana dashboard requirer (#117)
* Fix labels (#120)
* Use cosl for Rules and JujuTopology (#121)
* Fix position files (#116)
* top level documentation for cos_machine lib  (#122)
* Add juju-info itest (#123)
* Snap plug logs (#124)
* Loki shared logs monitoring (#125)
* Rename cos-machine to cos-agent (#129)
* wrong scope for grafana-dashboards-provider relation (#134)
* FIX Grafana dashboards files are stored in a bad format (#139)
* move on-refresh config to unit databag (#142)
* Set BlockedStatus per relation (#141)
* Be looser with finding snap plug mounts (#145)
* Use an actual uuid4, alerts() is now a property everywhere (#151)
* Replace DS_PROMETHEUS with prometheusds (#148)
* Cleanup (#152)
* use better locations for the positions files (#155)
* added typing for metrics endpoint dict (#149)
* Fix fstab parser (#159)
* (cos-agent) Use peer relation data for communicating all principals' data (#160)
* add support for the grafana cloud integrator charm (#150)

Co-authored-by: Dylan Stephano-Shachter <dylan.stephano-shachter@canonical.com>
Co-authored-by: awnn <adrian@webyda.se>
Co-authored-by: Ryan Barry <ryan.barry@canonical.com>
Co-authored-by: PietroPasotti <pietro.pasotti@canonical.com>
Co-authored-by: Jose C. Massón <939888+Abuelodelanada@users.noreply.github.com>
Co-authored-by: sed-i <82407168+sed-i@users.noreply.github.com>
Co-authored-by: Simon Aronsson <simme@arcticbit.se>
Co-authored-by: Luca Bello <36242061+lucabello@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(machine_charm) When related over loki_push_api, grafana-agent fails to start
4 participants