Skip to content
This repository has been archived by the owner on Oct 20, 2023. It is now read-only.

Add support for routes #129

Closed
wants to merge 1 commit into from

Conversation

adleong
Copy link
Contributor

@adleong adleong commented Mar 20, 2020

Some resources may break down their traffic metrics into logical groupings of
requests called routes. For example, a web application might break down metrics
by request path. If the route field is set, the traffic metrics apply to that
route only. If the route field is empty, the traffic metrics apply to the
resource as a whole.

Copy link
Contributor

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

Can you please bump the API version to v1alpha3 and squash all commits.

@stefanprodan
Copy link
Contributor

Looks like we have a version mismatch, in the SDK the metrics API is at v1alpha2 and in the spec at v1alpha1 😱

cc @grampelberg

@adleong
Copy link
Contributor Author

adleong commented Mar 20, 2020

Hey @stefanprodan. I've squashed the commits as you requested. If I'm understanding correctly, smi-metrics in the smi-spec is currently at v1alpha2. Or, at least that's the version listed at the top of traffic-metrics.md; is there somewhere else I should be looking?

It was my intention that this change and all the previous ones I've made to the spec (e.g. support for specifying the "side" and support for traffic splits) would be part of v1alpha2. It doesn't seem reasonable to me to bump the version for each spec PR. We could probably benefit from some concept of cutting releases of the spec so that we can intentionally bump the version when appropriate. But perhaps I'm missing some context for the way this API versioning was intended to work. Please let me know what you think.

@stefanprodan
Copy link
Contributor

is there somewhere else I should be looking?

In your example it's apiVersion: metrics.smi-spec.io/v1alpha1

It was my intention that this change and all the previous ones I've made to the spec (e.g. support for specifying the "side" and support for traffic splits) would be part of v1alpha2.

Got it, I haven't made the connection to the previous PR. Maybe an issue that would link both PRs would've been better.

We could probably benefit from some concept of cutting releases of the spec so that we can intentionally bump the version when appropriate.

Yes we should discuss this at the next meeting.

Copy link
Contributor

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

Can you update the examples from apiVersion: metrics.smi-spec.io/v1alpha1 to apiVersion: metrics.smi-spec.io/v1alpha2?

@adleong
Copy link
Contributor Author

adleong commented Mar 25, 2020

Ah, you're right, I missed updating one of the exampled. Fixed.

@stefanprodan
Copy link
Contributor

@adleong I was referring to metrics.smi-spec.io/v1alpha1 but you've changed specs.smi-spec.io/v1alpha1

@adleong
Copy link
Contributor Author

adleong commented Mar 25, 2020

@stefanprodan Gah, you're right, sorry. Technical difficulties on my end :)

Should be all fixed up now!

stefanprodan
stefanprodan previously approved these changes Mar 25, 2020
Copy link
Contributor

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Before merging I would squash all commits.

@adleong
Copy link
Contributor Author

adleong commented Mar 26, 2020

Squashed!

stefanprodan
stefanprodan previously approved these changes Mar 26, 2020
michelleN
michelleN previously approved these changes Jul 16, 2020
Copy link
Contributor

@michelleN michelleN left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you. Would you mind rebasing?

Signed-off-by: Alex Leong <[email protected]>
@adleong adleong dismissed stale reviews from michelleN and stefanprodan via 3727110 July 23, 2020 18:22
@adleong adleong requested a review from ilevine as a code owner July 23, 2020 18:22
Base automatically changed from master to main February 3, 2021 20:45
@bridgetkromhout
Copy link
Member

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

Successfully merging this pull request may close these issues.

4 participants