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

Split metrics on last client write #116

Merged
merged 5 commits into from
Jun 4, 2020
Merged

Split metrics on last client write #116

merged 5 commits into from
Jun 4, 2020

Conversation

matte21
Copy link
Collaborator

@matte21 matte21 commented Jun 1, 2020

This PR is the first step towards the changes discussed in #111 .
It splits metrics on the latencies of the lifecycles of NetAtts and subnets (NAs) on the basis of the last client precursor.

This PR self-contained and mergeable, but it's partial. It does not take into account in any way the fact that client precursors could be deletions of API objects as well (as pointed out here). It does not take into account the fact that implementation of API objects could be delayed because controllers could fail. Let's merge this and do remaining things in follow-up PRs.

Record virtual network relevance time in a more precise way:

1. When doing a sync with pre-existing network interfaces at start
up, correctly pick the earliest relevance time rather than the one
associated to the first local attachment to be processed.

2. Add a prometheus gauge to track the delay of the recorded
relevance time wrt the real relevance time.

3. Log a warning when a relevance delay is detected.
staging/kos/pkg/controllers/ipam/ipam.go Outdated Show resolved Hide resolved
staging/kos/pkg/controllers/ipam/ipam.go Outdated Show resolved Hide resolved
staging/kos/pkg/controllers/ipam/ipam.go Outdated Show resolved Hide resolved
staging/kos/pkg/controllers/ipam/ipam.go Outdated Show resolved Hide resolved
staging/kos/pkg/apis/network/types.go Show resolved Hide resolved
Follows a list of changes.

Rename: "creat" -> "create".

Access to Prometheus histograms and counters: from "With(...)" ->
"WithLabelValues(...)".

Refactorings to streamline code and reduce duplication.

Better wording of comments on Prometheus metrics.

Enforce consistent pass order of related function parameters.

Print VNI consistently in hex in logs and error messages.

Get rid of uninteresting Prometheus metrics.
return
}

func getAttLastClientWriteInfo(att *netv1a1.NetworkAttachment, naLastClientWrVal, naSubnetLastClientWrVal string) (lastClientWr string, lastClientWrTime time.Time) {
Copy link
Owner

Choose a reason for hiding this comment

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

The name of this func, "getAttLastClientWriteInfo", when compared with the preceding "getLastClientWriteInfo", differs only in the presence of "Att" --- which does not identify the difference between these two funcs. Perhaps this one should be named something more distintive. Perhaps something like "lastClientWriteInfoSwitch"?

Copy link
Collaborator Author

@matte21 matte21 Jun 4, 2020

Choose a reason for hiding this comment

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

The reasoning behind the name "getAttLastClientWriteInfo" is that it returns the last client write concerning an attachment, whereas the enclosing func returns the last client write among a group of writes that is related not (necessarily) just to the attachment being processed.

Copy link
Owner

Choose a reason for hiding this comment

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

Both are rooted at the NA, and even getAttLastClientWriteInfo looks further.
It is not just how far the func looks, but getAttLastClientWriteInfo takes parameters to use in characterizing the answer.

@MikeSpreitzer
Copy link
Owner

We can improve that func name later.

@MikeSpreitzer MikeSpreitzer merged commit 2687d4c into add-kos Jun 4, 2020
This pull request was closed.
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.

2 participants