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

Add ability to note client library/tool identity for usage metrics #91

Closed
grayside opened this issue Mar 11, 2019 · 21 comments
Closed

Add ability to note client library/tool identity for usage metrics #91

grayside opened this issue Mar 11, 2019 · 21 comments
Labels
kind/feature New feature or request lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Comments

@grayside
Copy link

Expected Behavior

Provide a standard mechanism for tools and libraries to self-report their identity and version. This will allow platforms built on knative to identify what developers are using to deploy services.

Actual Behavior

None

@steren

@steren
Copy link

steren commented Mar 11, 2019

+1 to the proposal.

Using a standard annotation that well behaved clients would populate seems to be a good fit.

CC @sixolet

@mattmoor
Copy link
Member

I don't completely grok what this is after from the terse explanation. We already report the Knative version several ways on system objects (via a label on resources, and a decorationin logs) and have talked about more (e.g. exposing via metrics endpoints).

What information is it that you want captured and how do you want it captured?

@steren
Copy link

steren commented Mar 11, 2019

We expect many Knative clients to be available.

The idea is to offer a standardized field (or annotation) that clients can optionally use to capture which "client was used to create this revision".

As a platform provider, we are interested in knowing which clients are most used by developers, or to be able to attribute usage to certain clients (e.g. usage of CLI vs UI).

As a developer, you might want to know if a given revision was created from a CI/CD system, or if it was created by a command line invocation. This can be achieved if both the CI/CD system and the CLI client populate this field.

I would expect this field to be a free form string. Clients could set it to an identifier, for example kn-0.1.0

@rgregg
Copy link

rgregg commented Mar 18, 2019

I think as a operator of the platform, this is important data. I'm tagging this for 0.6.

@mattmoor
Copy link
Member

@sixolet @cppforlife do you have thoughts on this?

@rhuss
Copy link
Contributor

rhuss commented Apr 30, 2019

+1 for an (optional) standard annotation. Optional, as it's set from the 'outside' and I'm not sure whether it's a good style to require mandatory, client-provided annotations.

An alternative would be a dedicated metadata field which would require to extend ObjectMeta. Not sure if this is something people want.

The third alternative would be to add to spec:. However, having client-provided meta-data in this section which does not add to the description of a target state is IMO misplaced.

@sixolet
Copy link
Contributor

sixolet commented Apr 30, 2019

I'd add a standard annotation for this!

@jchesterpivotal
Copy link

Isn't this what User-Agent is for?

@steren
Copy link

steren commented Apr 30, 2019

Based on our experience with Cloud Functions and Cloud Functions for Firebase, we learnt that user agents do not work well:

  • When calling the API from a browser, the browser's user agent is captured, instead of capturing "Google Cloud Console" for example.
  • We need this data to be stored as a property of the revision object, along with the deployedBy, to allow our developers to answer questions like: "What tool was used when person X deployed this revision at this time"

@jchesterpivotal
Copy link

All fair. I'd still agitate for sensible User-Agent values, but I can accept them as separate from this issue. Identifying badly-behaved clients presenting themselves as Go-http-client/1.1 or Ruby has a certain tooth-extractive feel to it that I resent.

@mattmoor
Copy link
Member

mattmoor commented May 1, 2019

+1 to different user agents, but that's probably not tenable for us to act on from within the webhook where the user-agent is likely Kubernetes.

Should we move this sort of thing to the Client repo moving forward?

@grayside
Copy link
Author

grayside commented May 1, 2019

Does the client repo set the expectations for unrelated client libraries?

@mattmoor
Copy link
Member

mattmoor commented May 3, 2019

I think knative/clients should own any sort of standardization/conventions for how clients interact with our APIs.

@mattmoor mattmoor transferred this issue from knative/serving May 3, 2019
@steren
Copy link

steren commented May 3, 2019

I do not understand why this has been transferred. Don't we need to agree on an annotation or API field in knative/serving and capture it in the spec?

@evankanderson
Copy link
Member

Should Audit Policy be sufficient for this?

https://kubernetes.io/docs/tasks/debug-application-cluster/audit/#audit-policy

User-Agent was added here: kubernetes/kubernetes#64791
Which was fixed here: kubernetes/kubernetes#64812 for 1.12

@steren
Copy link

steren commented May 3, 2019

@evankanderson : it would be great if the client name was captured at the revision level, so that developers could easily see which tool was used to create a certain revision. Instead of having to leverage an audit log. What do you think?

If we go with a standard annotation (as approved by @sixolet):

I suggest client.knative.dev/name. And the value should be a name that stays the same for a given Knative client. For example, kn could use kn.

Optionally, we could also add a client version with client.knative.dev/version. the value would depends on the client It could be something like 1.2.0

@evankanderson
Copy link
Member

The client team could choose to perform this as an extension, but I don't think that most Kubernetes tools will know anything about it, which is why I pointed out Audit Logs as an existing place this is recorded.

If you want to cooperatively record the sending system in the annotation, I don't object, but I don't think the serving API implementation has much it can do here.

@grayside
Copy link
Author

grayside commented May 3, 2019

What's called for here is:

  • A consistent way that anything deploying a revision can declare what tool/service was responsible for that deployment
  • A consistent way to find that information starting with a revision.

if the knative/client repo is responsible for the "API program", this makes sense. If knative/client is responsible only for the code & tools created by this community it does not.

Is the lack of support by most kubernetes tools a suggestion this be addressed in kubernetes community first?

@evankanderson
Copy link
Member

The lack of support by Kubernetes tools today is an observation. Another observation is that many users may be interacting with the Knative APIs via tooling which is Kubernetes-focused, such as Teraform, Kubectl, Helm, etc.

If there is a requirement that these kubernetes-but-not-knative tools be individually-identifiable, then there needs to be a plan which makes this possible. At the API level, our tool would be MutatingAdmissionWebhook, which receives an AdmissionRequest which does not contain the user agent. If you need user-agent type information for all clients, you would need to make a KEP similar to the changes I highlighted in #91 (comment).

@evankanderson
Copy link
Member

FWIW, I definitely buy the "cluster operators need to be able to determine the software used against the cluster (but not necessarily at low cost)" argument. I'm not sure that I buy the "developers need to know the software used to deploy a revision at high fidelity and low cost" argument.

The audit logging features I mentioned above are available to operators, but not to developers. Using an Knative-specific annotation provides a low-cost feature available to developers, but does not provide high fidelity (teraform, helm, kubectl, kustomize, etc will all appear as "unknown" clients).

@sixolet sixolet added the kind/feature New feature or request label Jul 11, 2019
@github-actions
Copy link

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 15, 2020
dsimansk added a commit to dsimansk/client that referenced this issue Feb 28, 2023
* Update spec file to release 1.6

* Cherry-pick test fixes, bump to client v1.6.1

---------

Co-authored-by: David Simansky <dsimansk@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

No branches or pull requests

8 participants