-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
plugins: set OTEL_RESOURCE_ATTRIBUTES when invoking a plugin #4875
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #4875 +/- ##
==========================================
- Coverage 61.31% 61.24% -0.08%
==========================================
Files 287 287
Lines 20065 20090 +25
==========================================
+ Hits 12303 12304 +1
- Misses 6869 6893 +24
Partials 893 893 |
I'm wondering:
|
I agree with @laurazard, to me this feels a bit hacky. I don't have an alternative solution for this right now, but we should think about this a bit. |
Oh! Forgot to comment; yes agree as well. If we want to fix this, we should at least not make this specific to "build", as there's other plugins that may have the same / similar issues. |
Very quick blurb / thought; I think we currently alias all of these to the same command in
Would it make sense to have the plugin (
Having such aliases also allows the plugin to apply specific behavior as well for each. In most cases this may only be to "adjust Perhaps the above would also need / could also need the plugin to advertise commands and aliases it wants to register as part of its metadata, e.g. something like; ~/.docker/cli-plugins/docker-buildx docker-cli-plugin-metadata
{
"SchemaVersion": "0.1.0",
"Vendor": "Docker Inc.",
"Version": "v0.12.1-desktop.4",
"ShortDescription": "Docker Buildx",
"aliases": {
"docker build": "build",
"docker builder": "builder",
"docker image build": "image build"
}
} |
I have two main thoughts regarding this:
Maybe i'm missing something, but I really wouldn't make |
Yes, that was another approach I was considering. Only part there would be that it would (currently) be breaking backward-compatibility with existing plugins, so we'd need some way for the plugin to advertise that it supports that (to prevent it failng with A potentially nicer approach would be to start using the socket we now provide as a communication channel (i.e. the plugin could request this information from the CLI when needed).
One thing could be to print the correct "usage" output, for example, any of the below show the same usage output, but none of thm used docker build --help
Usage: docker buildx build [OPTIONS] PATH | URL | -
...
docker image build --help
Usage: docker buildx build [OPTIONS] PATH | URL | -
...
docker builder build --help
Usage: docker buildx build [OPTIONS] PATH | URL | -
...
Possibly, there would bee other things to modify there, for example, it doesn't show available aliases ( docker build --help
Usage: docker buildx build [OPTIONS] PATH | URL | -
Start a build
Aliases:
docker buildx build, docker buildx b I think that may have been added by maintainers who didn't like typing much. Perhaps that's ok when using
Definitely would need discussion. My thinking here is also for a plugin to be able to register aliases under existing "management" subcommands. Currently plugins can only add a top-level command (which usually would be a group of commands), i.e. the For some plugins we may consider them to be able to register additional functionality in parts of the CLI, e.g., think of a
Yeah, I agree in general; there's been a ton of discussion about that. Not sure from the top of my head now what the exact behavior is right now; but read the discussion on #3676 for more details. Still there could be cases where things may be different depending on how they're invoked; possibly the same "plugin" command could back multiple things, but needs to adapt behavior (perhaps not a good example, but a bit similar to #3265 - all of which could theoretically be backed by a I think the |
Just throwing some ideas out there, I don't want to block anything
Confusing for who? I might be wrong, but IIRC most users should not really be aware that the CLI itself has plugins which should mostly behave like the standalone binaries, so I think the confusion is mostly for those people who already know the inner workings of the code a bit.
Thinking of one of our goals (making the CLI UX more consistent and generally better), are we sure each individual plugin should handle these sorts of messages for the user in its own way? It seems to me that if we could have a communication channel between plugins and the CLI (like the aforementioned socket), then the plugins could delegate some of those user-facing behaviors to the CLI itself (so we could for example unify error/warning messages and their styling, the plugins wouldn't need to know about aliases, we could have better control of the general UX, etc). This could clearly also be achieved if "everyone is on the same page" and implements user-facing behavior in the same agreed-upon way.
Now this example is getting me thinking..I guess it could make sense for the plugins to register their own aliases when used by the CLI, and it shouldn't even be too much work once we decide on how we'd like to persist some data for the CLI (eg. in a sqlite db we could also use for the metadata caching). Maybe something along the lines of:
|
The primary reason is because we are able to collect much more detailed metrics inside of the plugin, but we still want to know how it was invoked. If there's maybe a more acceptable way, OTEL itself has a way of overriding the service name used for a resource with an environment variable. Would doing something like setting I don't know if we would need anything else beyond that. I don't think it's a good idea to allow plugins to register aliases with their metadata and I also don't think it's necessarily a good idea for the plugin to be able to meaningfully change it's behavior based on the alias that was used to invoke it. |
Yes, so that part is where things currently break, because the
Currently, at least they do, because the CLI itself doesn't know what the plugin provides, and the plugin may have output to produce as part of its invocation (including
This would need to be looked into. I also want to avoid having to hard-code every alias for each plugin in the CLI, because that would defeat the whole purpose of plugins to allow them to be developed separate from the
Yeah, so they already do (which was the whole discussion about that I linked to). That is, unless you ran i.e.
But
I'm curious where OTEL should be produced for the invocation. I wonder if we'd end up with things being double (i.e. |
At least for this implementation, we're not utilizing spans so there isn't really a concern of measuring two spans. We're using the metrics side of OTEL so that we can constrain a bit more easily the data we collect and prevent sending too much to the docker backend which is a black box to people outside of docker. For the double span thing, I'll likely want to take a look at it to understand it, but if we were using the spans, we'd be looking for one specific span which would be the one that buildx produces. In order to have an issue with a double span, we'd need to invoke the same code twice and produce the same span. At the moment, we're mimicking what compose does for its implementation. The compose plugin (not For the Doing it in this way scopes the change to only OTEL and seems a bit less hacky than just a random environment variable name. It might also help OTEL traces to be a bit more fully fledged since these names will show up in the traces so a person looking at the trace will be able to see them the same way the metrics endpoint sees them. |
Updated the PR to use the service name method I was referring to since I think that method would be desirable for us and provide some more uniformity within the CLI. @milas just want to check that doing this doesn't cause any conflict with compose since this uses a similar path. I believe this should just ensure that the service name is properly set to |
Also, if we don't want to overload service name, we can choose our own attribute names such as |
Late to the discussion (or early, but I missed the follow ups from everyone 😅 A few points, top down, as I'm reading through the discussion that happened in the meantime:
Metrics, to provide correct usage information ("I ran It's very important to remember that most (all?) CLI plugins currently, explicitly, support both being executed as a CLI plugin or as a standalone binary, and as such in general do care about how they are invoked, and should behave properly in both cases.
Same point as above. Actually, for historical reasons, some users expect that plugins such as Compose are a standalone thing, and are still executing
It's complicated. On one hand, there is a lot of stuff that we could (and indeed, that was the idea) use the socket for! Even more, if you look at the hooks PR #4376, the idea was literally to take a structure logging approach to things (we discussed internally later using go templates, or something else, this was just a PoC) and let the CLI handle the actual printing of messages. However, we also probably want the plugins to have the same output whether connected to the CLI or not, so we can't abstract all of that away from the CLI. What we could do is build libraries for printing these kinds of messages/a bunch of other things into the CLI that the plugins can use, whether running as a CLI plugin or not, to print messages, which would achieve the UX consistency goal.
Yeah! The system you describe here is the same system as we discussed when we talked about carrying @neersighted's work with on-disk plugin metadata caching. In reality, you're mixing two concerns here – performance, and plugin aliases/etc – that is, on-disk caching is entirely orthogonal (but is recommended and we should follow up on it) for us to let plugin configure their own aliases, which could all be done at runtime. I'll leave the OTEL discussion to those in the know, but from what I could see before, I don't think we should have double-span issues between the CLI invocation and the plugin's own. |
From my uninformed perspective, I would think that the CLI-plugin architecture would follow a client-server design, with the socket (as @laurazard and @thaJeztah mentioned) being the communication channel. There are of course other methods of IPC, but sockets are easy for plugins to adopt. My thoughts on this are similar to @krissetto that you would have some sort of common interface of communication between the CLI and each plugin. I suppose to get something that is language agnostic, one could go with something like protobuf and have the CLI create the spec which plugins can consume in a defined way. The CLI could then provide certain functionality to plugins (such as logging message etc.). Of course as @thaJeztah and I discussed this could be difficult since some plugins wouldn't want to be fully dependent upon the CLI for certain features, such as logging :). Maintaining the CLI would also become more difficult as the features the CLI exposes to plugins increase. The question of some plugins wanting to be independent from the CLI as well as dependent makes it tricky. I suppose there are ways around this by having a wrapper around the standalone to make it plugin compatible or as @thaJeztah mentioned to me - compiling with flags to set the binary in a plugin mode could also work. TL;DR plugins are hard and making everything work would take some thought. I agree to use the socket for IPC. |
For the purposes required for what we need, I think a bidirectional socket communication might be too complicated. We don't really need to have a code path that asks the CLI for any information. We just mostly need it to tell us the command the plugin was originally invoked with. I'm not sure what's the best way, but I do think likely using
The advantage of using The advantage of option 1 is that we are using our own namespace and it won't conflict with any existing patterns. It's also straightforward and gives the exact information I need for my use case without becoming too much code. The advantage of 2 is it gives more information in an already standardized way, but I'm afraid that it could potentially conflict with the plugin itself since the values will be for the CLI process and not the plugin. The 3rd option has the advantage of being standardized for naming, but also scoped so the CLI process and plugins don't compete with each other and gives more information than option 1 in a bit more of a generic way. |
ef2e883
to
9f9a850
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ok to me.
I haven't tested it but I like the idea here.
9f9a850
to
77fa945
Compare
I've made some modifications from the original and wanted to go over them along with the ideas I considered but didn't like for one reason or another. I tried using the existing I figured that using those and recording the resource at the start of the program (before any of the aliases has changed the command args) and then passing them to the plugin would work. It did, but it also came with the side effect that every plugin received the command line arguments in plaintext as part of the resource attributes in an environment variable. This included any sensitive information such as passwords from the command line and that would mean that the resource on traces and metrics would all have potentially sensitive command line arguments. This seemed too much. There wasn't another already existing OTEL attribute that would only give the subcommands and I figured adding one wouldn't make sense because there was too large of a possibility there would be a future conflict and, from my understanding, the My next attempt was to use CommandPath from Cobra. In the most recent version of Cobra (not the one we're using), you can use an annotation to override the root command's output for the purposes of help messages. This functionality might be useful for some of the issues discussed in this issue regarding help messages. I ultimately decided against it for a few reasons.
Still, this annotation name For the implementation, I opted to create a new docker-specific annotation. If this annotation is set, it uses it without any additional processing. If it is not set, it defaults to |
df76528
to
99eb502
Compare
When a plugin is invoked, the docker cli will now set `OTEL_RESOURCE_ATTRIBUTES` to pass OTEL resource attribute names to the plugin as additional resource attributes. At the moment, the only resource attribute passed is `cobra.command_path`. All resource attributes passed by the CLI are prepended with the namespace `docker.cli` to avoid clashing with existing ones the plugin uses or ones defined by the user. For aliased commands like the various builder commands, the command path is overwritten to match with the original name (such as `docker builder`) instead of the forwarded name (such as `docker buildx build`). Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
99eb502
to
5786f20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"LGTM"; looks like many have this a review, and we agreed on this, so bringing this in.
- What I did
When a plugin is invoked, the docker cli will now set
OTEL_RESOURCE_ATTRIBUTES
to pass OTEL resource attribute names to theplugin as additional resource attributes. At the moment, the only
resource attribute passed is
cobra.command_path
.All resource attributes passed by the CLI are prepended with the
namespace
docker.cli
to avoid clashing with existing ones the pluginuses or ones defined by the user.
For aliased commands like the various builder commands, the command path
is overwritten to match with the original name (such as
docker builder
) instead of the forwarded name (such asdocker buildx build
).- How I did it
This sets the
OTEL_RESOURCE_ATTRIBUTES
environment variable when invoking plugins and provides an option for the CLI to override this name through an annotation.- How to verify it
See docker/buildx#2225.
- Description for the changelog
Set
OTEL_RESOURCE_ATTRIBUTES
when invoking plugins.- A picture of a cute animal (not mandatory but encouraged)