Skip to content

Modified service bindings to add route bindings#127

Merged
gmllt merged 10 commits intocloudfoundry:masterfrom
adamspd:PAASCFY-2654-service-binding-metric
Jun 18, 2024
Merged

Modified service bindings to add route bindings#127
gmllt merged 10 commits intocloudfoundry:masterfrom
adamspd:PAASCFY-2654-service-binding-metric

Conversation

@adamspd
Copy link
Contributor

@adamspd adamspd commented May 27, 2024

No description provided.

@adamspd adamspd marked this pull request as draft May 27, 2024 12:35
@adamspd adamspd marked this pull request as ready for review May 27, 2024 12:49
@adamspd adamspd changed the title Modified service binding to add route binding Modified service bindings to add route bindings May 27, 2024
gmllt
gmllt requested changes May 30, 2024
View reviewed changes
Copy link
Member

@gmllt gmllt left a comment

Choose a reason for hiding this comment

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

I think fetch should go through its own fetch function fetchServiceRouteBinding in order to take maximum advantage of the parallelization offered by the worker system.

@adamspd adamspd requested a review from gmllt May 30, 2024 09:58
@gmllt
Copy link
Member

gmllt commented May 30, 2024 •
edited
Loading

@adamspd Sorry, I was so wrong when we discussed the usefulness of a new metric.
Now, I see the benefit to introduce a new metric only for service_route_bindings, as service instances bound to a route are not bound to any app at the same time.

You were right, it is better to introduce a new metric to avoid the emergence of service_bindings metric which would not contain an application_id

@adamspd adamspd force-pushed the PAASCFY-2654-service-binding-metric branch from 04fd78c to bfc82ba Compare June 18, 2024 08:03
gmllt
gmllt approved these changes Jun 18, 2024
View reviewed changes
Copy link
Member

@gmllt gmllt left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @adamspd

@gmllt gmllt merged commit 82b5d0b into cloudfoundry:master Jun 18, 2024
@benjaminguttmann-avtq
Copy link
Contributor

benjaminguttmann-avtq commented Jun 18, 2024

like mentioned in the other PR please make sure to run the tests before a PR gets merged

https://github.com/cloudfoundry/cf_exporter/actions/runs/9561354046/job/26355377312

@adamspd
Copy link
Contributor Author

adamspd commented Jun 18, 2024

like mentioned in the other PR please make sure to run the tests before a PR gets merged

https://github.com/cloudfoundry/cf_exporter/actions/runs/9561354046/job/26355377312

I should've put the PR in draft, I was about to add it just before the merge. Thanks for the reminder.

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

Reviewers

@gmllt gmllt gmllt approved these changes

Assignees

No one assigned

Labels

None yet

Projects

Foundational Infrastructure Working Group
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@adamspd @gmllt @benjaminguttmann-avtq