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

Open charts from tooltips #20

Open
arendjr opened this issue Apr 26, 2023 · 14 comments
Open

Open charts from tooltips #20

arendjr opened this issue Apr 26, 2023 · 14 comments
Labels
enhancement New feature or request
Milestone

Comments

@arendjr
Copy link
Collaborator

arendjr commented Apr 26, 2023

Our Autometrics tooltips contain queries. Currently, those point to external URLs, but when we have charts inside the extension, we want to open them right there for a consistent experience.

One complicating factor is that different languages have different mechanisms for opening tooltips. Some rely on the extension itself, which is likely easier to intercept, but for instance Autometrics-rs generates Rustdoc strings, which we may need to detect somehow.

Charts should consistently open within the extension, regardless of language.

@arendjr arendjr added the enhancement New feature or request label Apr 26, 2023
@arendjr arendjr added this to the 1.0 milestone Apr 26, 2023
@arendjr
Copy link
Collaborator Author

arendjr commented May 1, 2023

I'm trying to figure out how to use the Autometrics hover tooltips to open their queries inside the VS Code extension's chart panel. My idea was the extension should just intercept clicks to the Prometheus URL and handle them internally, but as far as I can find, VS Code actually won't let you do that. The only URLs you can handle are those with a special prefix that is registered by the extension, which means that -- in case of the Rust implementation -- the doc string generated by the macro must generate URLs with the VS Code extension prefix in them. Maybe this is not a huge issue, because users can just set their PROMETHEUS_URL to match the extension prefix, and that way it should work. It very much feels off though, and not a great DX. It also means people need to check that workaround into their repository, forcing teams to either commit everyone to using the extension -- or no one.

@emschwartz can you maybe think think of a way how we might be able to let the extension inject the prefix into the macro config?

@emschwartz
Copy link

emschwartz commented May 1, 2023

Hmm.

Just brainstorming here but these are the things that come to mind:

  • Either we need to have the extension catch the normal URLs, or we need to modify the URL before it's generated
  • One maybe crazy option for catching the normal URLs would be to actually run a server on localhost:9090 and redirect to the extension URLs. That's not ideal though because it wouldn't work if you did actually have Prometheus running locally (though most people won't have that, in which case it wouldn't matter)
  • If we want to change the URLs that are generated, we can either a) tell users to modify their PROMETHEUS_URL env var b) have the extension modify that env var c) configure the library to look for something other than an environment variable that the extension could modify
  • I don't know if it's possible for the extension to modify the PROMETHEUS_URL env var because that would need to be passed into rust-analyzer and I'd be somewhat surprised if VS Code let one extension modify the config of another
  • Maybe it would make sense to look for this URL in some file in the project directory... 😕 I don't like that very much but I'm wondering if there's any other way to have the extension "communicate" with the library

@arendjr
Copy link
Collaborator Author

arendjr commented May 1, 2023

Yeah, I'm afraid letting it modify env vars used by rust-analyzer isn't really feasible indeed.

Looking for it in a file was also a last resort I thought of, though not really desirable since people will most likely want to put it in their .gitignore then. Not a dealbreaker maybe, but still annoying. But I admit so far I cannot really think of something better 🤷

@emschwartz
Copy link

This seems somewhat related (but unfortunately it's unresolved) microsoft/vscode#152806

@arendjr
Copy link
Collaborator Author

arendjr commented May 1, 2023

rust-analyzer apparently also has its own API for setting environment variables, though the way it is worded seems to imply it only applies to their debugger/launch configurations: https://rust-analyzer.github.io/manual.html#setting-runnable-environment-variables

Internally, they even seem to have an option for setting extra environment variables as well: https://github.com/rust-lang/rust-analyzer/blob/master/crates/rust-analyzer/src/config.rs#L108
But so far I haven't been able to find if or how they expose that from the extension. From what I gather so far it's only accessible if you talk to the language server directly, instead of using the extension.

I do suppose we could ask them if they would accept a PR to open up this functionality...

@arendjr
Copy link
Collaborator Author

arendjr commented May 1, 2023

I tried setting both of these in settings.json:

"rust-analyzer.cargo.extraEnv": {
    "rustc-env": "PROMETHEUS_URL=vscode://"
}

and:

"rust-analyzer.cargo.extraEnv": {
    "PROMETHEUS_URL": "vscode://"
}

But no dice...

@arendjr
Copy link
Collaborator Author

arendjr commented May 1, 2023

Bingo, it's actually this one:

"rust-analyzer.server.extraEnv": {
    "PROMETHEUS_URL": "..."
}

I will first try if I can really catch it that way. But then we just need to figure out if we can set that setting programmatically, or if we just need a bit more elaborate setup instructions.

Another complication is that the PROMETHEUS_URL setting in build.rs takes precedence, so assuming people have set things up that way, they also need to make another adjustment there...

@arendjr
Copy link
Collaborator Author

arendjr commented May 1, 2023

@emschwartz I suppose we could do a lookup for EDITOR_PROMETHEUS_URL with a fallback to PROMETHEUS_URL, right? That way we could at least let people configure the extension (or let the extension configure it for you) without conflicting with the build.rs.

@arendjr
Copy link
Collaborator Author

arendjr commented May 1, 2023

Ran into another snag. My extension is now able to catch the URI vscode://fiberplane.autometrics/, for instance vscode://fiberplane.autometrics/graph?g0.expr=.... This works if I write a Markdown file myself and click on the link. But somehow the hovers displayed by the editor refuse to make such links clickable. They just don't become links. Neither when generated by Autometrics, nor when I write a doc comment at a function myself :(

@arendjr
Copy link
Collaborator Author

arendjr commented May 1, 2023

One thing that does work is this:

vscode.workspace
    .getConfiguration("rust-analyzer")
    .update("server.extraEnv", {
      PROMETHEUS_URL: "vscode://fiberplane.autometrics",
    });

@arendjr
Copy link
Collaborator Author

arendjr commented May 2, 2023

I have opened an issue with VS Code to ask for suggestions on how we might be able to intercept links (or whether they might be open to implementing such a feature): microsoft/vscode#181320

@arendjr arendjr modified the milestones: 1.0, 1.0-beta May 3, 2023
@flenter
Copy link
Member

flenter commented Jun 22, 2023

Right now we've got support for:

  • python
  • rust (but only basic)
  • typescript (is blocked by an issue when using the bundled autometrics typescript plugin in combination with typescript 5). Will be fixed with the Update hover tooltips for typescript  #49 PR

@emschwartz
Copy link

@flenter what's the basic part about the Rust support? Anything you need from the library side?

@flenter
Copy link
Member

flenter commented Jun 22, 2023

I was under the assumption that the currently implemented logic for rust was actually detecting whether the function is using the autometrics trait. But that's not the case, the extension right now assumes that all functions have been decorated.

It seems like there's more stuff to be done here.

Rust support should probably be disabled by default, if we want to ship the extension or start working on a solution similar to what's discussed here: #29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants