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

Expand wildcard targets into individual jobs #370

Merged
merged 9 commits into from
Sep 29, 2022
Merged

Conversation

sed-i
Copy link
Contributor

@sed-i sed-i commented Sep 23, 2022

Issue

Config looks roughly like this:

- job_name
  scheme
  basic_auth
  metrics_path
  static_configs:
    - targets:
        - host:port
      labels

The scrape URL components are split into two hierarcical levels of a scrape_config:

  • top level:
    • scheme
    • basic_auth (part of netloc)
    • metrics_path (the suffix of the path; the path prefix will be coming from per-unit relation data)
  • inner level:
    • static_configs->targets->host:port (part of netloc)

Currently, "static_configs": [{"targets": ["*:1234"]}] is autoexpanded into

  'static_configs': [{'labels': {'juju_unit': 'remote-app/0', ...},
                      'targets': ['10.10.10.10:1234']},
                     {'labels': {'juju_unit': 'remote-app/1', ...},
                      'targets': ['11.11.11.11:1234']}]}]

Which means we can't specify different metrics_path per unit, but it is needed when using ingress-per-unit.

Solution

Expand wildcard targets into individual jobs

Context

#349

Testing Instructions

Release Notes

@sed-i sed-i force-pushed the feature/one_job_per_unit branch from 47fb0eb to bd03af3 Compare September 26, 2022 10:55
@sed-i sed-i changed the base branch from feature/fix-ingress-usage to main September 26, 2022 10:55
@sed-i sed-i force-pushed the feature/one_job_per_unit branch from bd03af3 to aaffe77 Compare September 28, 2022 11:54
@sed-i sed-i marked this pull request as ready for review September 28, 2022 13:56
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.

So far, this looks great. It doesn't actually cover the use case of the target having ingress-per-unit and requiring a different scrape endpoint per job (yet), but this is a good abstration and cleanup.

I wonder whether it would be better to set a per-unit metrics path or to simply set a flag that ingress-per-unit is enabled. Passing in an IngressPerUnitRequirer to MetricsEndpointProvider and checking the status of that to determine the path is a much simpler UX, since the "actual" PathPrefix is reasonably predictable, and could be constructed in the library so authors don't need to worry about how to create an external_url property or similar.

On the other hand, making it more flexible allows for easier upkeep if the IPU API ever changes.

@sed-i
Copy link
Contributor Author

sed-i commented Sep 29, 2022

I wonder whether it would be better to set a per-unit metrics path or to simply set a flag that ingress-per-unit is enabled. Passing in an IngressPerUnitRequirer to MetricsEndpointProvider and checking the status of that to determine the path is a much simpler UX, since the "actual" PathPrefix is reasonably predictable, and could be constructed in the library so authors don't need to worry about how to create an external_url property or similar.

  • I agree we construct & deconstruct url parts a bit too much. Rethinking this should probably be a v1 effort.
  • Introducing interlib dep. injection is worth exploring but also seems like a greater effort. For example adding a Protocol file to the lib interface repo for every relation. I'd like to stay away from this altogether under the current scope.

@sed-i sed-i mentioned this pull request Sep 29, 2022
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.

4 participants