Rename websocket notification channel URL path

[hareetd]

Change the path from api/v1/command to api/v1/notifications since the websocket channel is now used for one-way notifications instead of commands.

Fixes #478

[andrewazores]

How does this handle the Since it's part of the published 1.0 API, the existing path should still be exposed and result in a successful connection if clients attempt to use it. requirement from the original issue?

[hareetd]

Looks like I misunderstood the issue. Breaking that requirement down:

1. the existing path should still be exposed: does this mean the ClientUrlGetHandler should still return the path api/v1/command, or perhaps return both possible paths?

2. result in a successful connection if clients attempt to use it: I guess this means I should include functionality in the code such that both paths "work"

I'm just a bit confused because I thought with the deprecation of Commands we would want to remove any mention of them. Are we waiting to do that until the 2.0 API is published and completely replaces the 1.0 API?

[andrewazores]

the existing path should still be exposed: does this mean the ClientUrlGetHandler should still return the path api/v1/command, or perhaps return both possible paths?

It can just return the new path, as you've done. This is what clients are supposed to use to check what the notification channel URL is, so it should tell them what the newest/preferred place to go is. But, clients aren't required to do a GET on this endpoint before connecting - it's just there as a reference and the "correct" way to check where to connect to before opening a connection.

The handler may as well be renamed while we're here, too. ClientUrl reflects the original interactive mode from early ContainerJFR days. NotificationsGetHandler would match the handler naming convention better now.

result in a successful connection if clients attempt to use it: I guess this means I should include functionality in the code such that both paths "work"

Right. Even though we want to stop referring to the notification channel as a command channel, since that is now a misleading name, it should also be cheap and easy for us to still support clients that try to connect on the old path. If a redirect (ex. HTTP 301) can be used here then that's probably ideal.

I'm just a bit confused because I thought with the deprecation of Commands we would want to remove any mention of them. Are we waiting to do that until the 2.0 API is published and completely replaces the 1.0 API?

The Commands removal definitely won't be going into v1, since we already have a published 1.0.0 release, and so any further 1.x.y releases should avoid changing or removing anything that would break any potential end users' clients and scripts that exist and were built against the 1.0.0 API. Breaking changes like that should only arrive in 2.0.0 (or any other major version release). That said, where it's possible, it would also be nice of us to make migration from 1.0->2.0 as easy as possible, so that could mean doing things like HTTP 301 redirects on renamed paths, so that any third-party clients/scripts will continue to work but users will (hopefully) see the 301 status and realize that they're using something that is changed/deprecated and they should prepare for it to be removed or otherwise more significantly altered (in a breaking way) in a future release.

[andrewazores]

On the other hand, since /api/v1/commands in 1.0.0 implements an interactive command channel, and /api/v1/notifications in 2.0.0 implements a push notification channel, perhaps /api/v1/commands should not redirect the client but instead respond with a 410 GONE response, so that the clients fail to connect and hopefully do so in an informative way for the end user.

[hareetd]

Thanks for the explanation, it cleared up any misconceptions I had.

I think a 410 GONE response would make more sense as it would (hopefully) funnel clients towards getting the new path from the updated handler. I'll go ahead and implement your other suggestions as well; will let you know if I have any other questions.

[hareetd]

@andrewazores (not sure if you get notified for comments on draft PRs)

I'm almost done but had a few questions first:

1. I decided to rename ClientUrlGetHandler to NotificationsUrlGetHandler, instead of NotificationsGetHandler as you previously suggested, because it matches the GrafanaDashboardUrlGetHandler convention and I think it's a more accurate name. Is this fine or should I change it to NotificationsGetHandler?

2. For the HTTP_API.md, should I also change the synopsis for the updated handler, and if so, what to?

3. Should I add a new test in MessagingServerTest to check if api/v1/command rejects the websocket connection with a 410 GONE status code? I'm leaning towards no since all the test would check is an if statement.

[andrewazores]

Yes, I still get notified on drafts.

1. No, what you've done makes sense and sounds good.
2. I think just removing/changing mentions of "command channel" to "notification channel" or similar should be fine?
3. A test probably does make sense even though it seems so simple. Doing it as an integration-test might be even better here.

[hareetd]

I've finished writing up the integration test; had a few difficulties extracting the HTTP status code from the thrown exception(s) so I settled with comparing the error message instead.

[andrewazores]

Looks good! Again, I will hold off on approving/merging this until there is a corresponding change in -web. Are you interested in taking a look at that yourself? It should be a pretty small patch.

[hareetd]

Sure, I can do that.

It looks like NotificationChannel.service.tsx in -web still references clienturl; I can open an issue and take care of that.

Were there any other corresponding changes you had in mind?

[andrewazores]

No, that's all that comes to mind.

[andrewazores]

web-client change is merged. Please update this PR with a commit bumping the submodule version again, and then I'll approve and merge this too.

[hareetd]

Just double-checking, but the merge conflict is because the web-client is pointing to a different commit than the one on the main branch, right?

[andrewazores]

Yes, something like that. I think the easiest way to resolve it would be to `git fetch upstream`, then `git rebase -i upstream/main` and drop your commit where you updated the web-client commit. This should remove the conflict. Then you can update the web-client and commit that again, and hopefully that would apply cleanly.

[hareetd]

Looks like it worked. I guess in the future it's best to rebase at the end to prevent merge conflicts? I've only been rebasing my branches right before opening the PR.

[andrewazores]

Usually you don't need to do a rebase before merging - if you do, you'll see the merge conflict status like it was here. This time it only happened because git couldn't figure out how to apply the different changes to the web-client from both of the changed branches. If your PR can be replayed on top of main, and there are either no conflicts or git can figure out how to resolve them automatically, then the pre-merge rebase is typically unnecessary. However, for a submodule commit hash change, there's no way to figure out how to merge those two changes - it has to just be one or the other, so we see the conflicting status.