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

Design API endpoints for v2 #1336

Closed
simitt opened this issue Aug 29, 2018 · 17 comments
Closed

Design API endpoints for v2 #1336

simitt opened this issue Aug 29, 2018 · 17 comments
Assignees
Labels
Milestone

Comments

@simitt
Copy link
Contributor

simitt commented Aug 29, 2018

Suggestion as a start point for discussions on how to design v2 API endpoints:

intake/v2/events/
intake/v2/rum/events/
assets/v2/sourcemaps/
/
@simitt simitt added the discuss label Aug 29, 2018
@watson
Copy link
Contributor

watson commented Aug 29, 2018

I would suggest /assets/v1/sourcemaps, but I'm fine either way.

@zube zube bot added the [zube]: Inbox label Aug 30, 2018
@watson
Copy link
Contributor

watson commented Sep 4, 2018

Besides what I wrote in my previous comment I'm 👍

@simitt
Copy link
Contributor Author

simitt commented Sep 4, 2018

@watson both prefixes assets and intake are new. Why would you use v1 for assets but v2 for intake? I suggest to be consistent to either use v1 or v2 for both.

Using v1 for both as it is both new endpoints, could be confusing as we already had another v1 endpoint for all the resources.
Thus I suggest to start with v2 for both.

@hmdhk
Copy link
Contributor

hmdhk commented Sep 4, 2018

@simitt , I think it should be like the following:

/v2/intake
/v2/rum/intake
/v2/assets/sourcemaps

Also, as far as I know the endpoints are unified (all event types go to the same endpoint), so there's no /events, isn't that right?

@beniwohli
Copy link
Contributor

Did the API for sourcemap upload change? If not, I'd try to maintain the existing URL as to not break any automation people might have set up to upload their sourcemaps on deploys.

Other than that I don't really have any preference

@simitt
Copy link
Contributor Author

simitt commented Sep 4, 2018

@jahtalab the suggestion I made with intake/v2/events and intake/v2/rum/events was a unified endpoint for all event types. It basically follows a {endpoint}/{version}/{resource} approach.

@beniwohli the sourcemap upload endpoint did not change. However, we currently have not distinguished intake api and uploading assets/resources to the server. As we deprecate the v1 for intake api endpoints, it might be confusing to have other endpoints of the same API not being deprecated.
By splitting up the API into intake and assets we avoid having to deprecate all endpoints in the future when we want to version bump a specific one.

@watson
Copy link
Contributor

watson commented Sep 4, 2018

@simitt You're right that we could technically just start over from v1 as the whole URL have changed anyway. But when talking about this we always call it v1 and v2, so it might be nicer to reflect that in the URL.

Regarding if intake and assets should have the same version: I thought the whole reason for splitting it up into two separate namespaces was so we didn't have to bump one version when we bumped the other. If not we could just keep the current URL's right?

@simitt
Copy link
Contributor Author

simitt commented Sep 4, 2018

@watson I suggested to use v2 and not v1, so seems we agree here.

I don't see consistency when moving from

/v1/transactions -> intake/v2/events
/v1/errors -> intake/v2/events
/v1/rum/sourcemaps -> assets/v1/sourcemaps

So my point was to either start counting at v1 or at v2 for the new APIs.

@hmdhk
Copy link
Contributor

hmdhk commented Sep 4, 2018

@simitt Thanks for clarification, I think having /events is unnecessary so to make my suggestion even simpler:

/v2/intake
/v2/rum
/v2/assets/sourcemaps

Regarding the versioning I think we should just use v2 and at some point deprecate everything under v1.

@watson
Copy link
Contributor

watson commented Sep 4, 2018

@simitt I'm fine either way 👍 From my point of view I don't think it's confusing to keep the assets version as v1 still. But if you feel it's better to start from the same version I'm cool with bumping it.

@jahtalab The reason why we want to move the version in one level is that we want to allow the versions to diverge. So we might have version 3 of the assets API but version 2 of the intake API. If the version is in the root as today this would be confusing to the user.

@hmdhk
Copy link
Contributor

hmdhk commented Sep 4, 2018

@watson, To be clear I think it's more confusing to have multiple API versions. That's why I suggested to just maintain one version of the API and deprecate the old one. If that would be the case then it make sense to have v2 in the root.

@watson
Copy link
Contributor

watson commented Sep 4, 2018

@jahtalab I see only two viable options (from a users point of view)

  1. Either have the version in the root and when the version changes it changes for all sub-paths as well. So /v2/intake would also mean /v2/sourcemaps
  2. Or separate the API's by a prefix like Silvia suggested

Personally I'm fine either way (but we should not keep it in the root like it is today and only bump the intake part).

But it seems to me that the server team do not want option 1 as it's actually two different API's with different update schedules. The intake API is under our control via the agents, but the source map API is not as users set up deploy scripts to upload the source map to this API. If we had to bump the asset version every time we bumped the intake version, it would add an extra burden on the users to update their deployment scripts to use the new endpoint.

@hmdhk
Copy link
Contributor

hmdhk commented Sep 5, 2018

@watson, the number 1 is basically what I suggested in my previous comment.

I think number 2 would just add additional confusion and makes it harder for us to maintain documentation. For example we need to keep a matrix of which versions of different apis exist on which versions of apm-server and which ones are deprecated.

Regarding the the burden of updating deployment scripts, there are two possibilities, either that API had some changes in which case they need to update their script to reflect those changes (which necessitates a new version), or it didn't have any actual changes which means they only have to update the prefix of the url.

@watson
Copy link
Contributor

watson commented Sep 5, 2018

@jahtalab I actually brought this up in an APM Server meeting recently. It might be helpful to watch that recording. But if you have time, let's just jump on a quick call and I'll be happy to discuss it in person - sometimes that's easier 😃

@hmdhk
Copy link
Contributor

hmdhk commented Sep 5, 2018

Had a chat with @watson IRL, I'm ok with the approach @simitt suggested, even though I think it's not the optimal solution.
However, I think we need to document the following information with each release of apm-server, (for the purposes of configuring proxies, firewalls, etc)

  • List of endpoints added
  • List of endpoints deprecated
  • List of endpoints removed

@roncohen
Copy link
Contributor

roncohen commented Sep 6, 2018

Great! Let's go with @simitt's suggestion:

/intake/v2/events
/intake/v2/rum/events
/assets/v2/sourcemaps

I suggest we keep, but deprecate /v1/rum/sourcemaps and /v1/client-side/sourcemaps for now and remove them in 7.0.

@jahtalab agreed that we should include documentation in the release notes whenever these change.

@roncohen
Copy link
Contributor

Closed by #1360

@zube zube bot added the [zube]: Done label Sep 12, 2018
roncohen added a commit to roncohen/apm-server that referenced this issue Oct 7, 2018
roncohen added a commit to roncohen/apm-server that referenced this issue Oct 15, 2018
roncohen added a commit to roncohen/apm-server that referenced this issue Oct 15, 2018
roncohen added a commit to roncohen/apm-server that referenced this issue Oct 15, 2018
roncohen added a commit to roncohen/apm-server that referenced this issue Oct 15, 2018
roncohen added a commit to roncohen/apm-server that referenced this issue Oct 15, 2018
roncohen added a commit to roncohen/apm-server that referenced this issue Oct 16, 2018
@alvarolobato alvarolobato added this to the 6.5 milestone Oct 26, 2018
bmorelli25 pushed a commit to bmorelli25/observability-docs that referenced this issue Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants