Restructure Pyroscope API into groups that can be enabled / disabled

[abeaumont]

Background

Pyroscope API has been growing over time in an unstructured manner, which will make it harder to maintain over time. Some of the problems it currently shows:

• There's not always a clear separation of the API. For example, the /ingestion endpoint is part of the API but doesn't have an /api prefix in the URI, as other API endpoints do.
• Most of the APIs have no versioning. With the exception of the adhoc API, our API has no versioning. This makes it really hard to evolve it.
• Most of the API is centralized. A single controller took care of most of our API, now we have a new API package that contains a set of independent APIs that are coupled in the same package, instead of being modular.
• There's no consistent way to enable/disable parts of the API. As the API grows, the exposure surface also grows, and there are whole API modules/components that people may not be interested in exposing (e.g the adhoc API, the users API, the ingestion API, etc.)

Proposed solution

Following the k8s terminology, I propose to use API groups instead, and provide a consistent mechanism to enable/disable these groups:

• Each component should provide its API group (instead of having an API package with all the components APIs). Gorilla mux makes this feasible.
• Each API group should be versioned.
• Each API group should be documented using OpenAPI.
• Each API group should be easily enabled/disabled in a consistent way.

The adhoc API shows how this could be achieved (API documentation missing).

[kolesnikovae]

I can't say I fully support the idea of modular API in pyroscope server.

Regarding API groups: although, I see some pros for us as maintainers of the API, I also see that for must API consumers this will be merely inconvenient in use (and often won't meet expectations). BTW, about the URLs: for groups it should be /apis/{group}/{version}, I believe.

Unrelated to API groups: I also think that disabling parts of the API should be discouraged. At least, endpoints should not return 404 in this case as it interferes with the handler logic.

Existing Controller should be definitely refactored. The way I picture it is described in the api package: to my mind, this is the most standard, explicit, and clear way - you may find hundreds of projects that adhere this approach (or similar).

[abeaumont]

Thanks for your feedback @kolesnikovae !

Regarding API groups: although, I see some pros for us as maintainers of the API, I also see that for must API consumers this will be merely inconvenient in use (and often won't meet expectations).

Can you elaborate on why this is going to be inconvenient and won't meet expectations? I understand that for an API user that would like to use all our API groups, it'd be easier to just have a single version for all our API, as they could just use a single client. Not sure if that's the inconvenience you are considering or there's something else. If this is the case:

• We may still have a single client with multiple API groups. This doesn't make sense in general (see next point), but it may make sense in some cases.
• The overlapping of the users that would want to use different API groups should be low, e.g. the ingestion API should be mostly used by the agent integrations, while the user API may be used either by Pyroscope itself or by some admin (and arguably, if we were to provide API clients, exposing the whole API to, say, the agent integrators, would be inconvenient).

Unrelated to API groups: I also think that disabling parts of the API should be discouraged. At least, endpoints should not return 404 in this case as it interferes with the handler logic.

Can you elaborate on what problems disabling endpoints causes? I can see some benefits to disabling endpoints (e.g. on a pull-based deployment not disabling the ingestion endpoint is inconvenient, same for exposing the adhoc API while it's disabled, etc.)

Existing Controller should be definitely refactored. The way I picture it is described in the api package: to my mind, this is the most standard, explicit, and clear way - you may find hundreds of projects that adhere this approach (or similar).

I have to admit that I left the details of the implementation of the controller out of the scope of this issue on purpose, as that's another huge topic to discuss by itself :). I agree that many (most) implementations work in this way. How we want to implement our API depends on what design we want to follow. If we want to have a monolithic design, that's a valid approach, but I do see value to have independent and unrelated components decoupled (e.g. having single api, model, etc. package that contains information about users, roles, apis, profiles, ingestion, adhoc, etc. is harder to maintain, in my opinion).

[kolesnikovae]

Let's consider topics separately, as they are really independent.

1. Versioning

We may need API versioning. I agree with the authors of the API versioning article and Roy Fielding (creator of REST):

You'll sometimes hear the advice that you must define a versioning strategy before you release your first version, or evolving your API will be impossible. This is not true.

You can always add a new versioning header later if you find the need to do format versioning and you can always add new URLs for new entities for a different entity version. Any requests that lack the format versioning header should be interpreted as meaning the first format version. Since instances of a new entity version get new URLs, you can easily introduce a version ID in those URLs without affecting the URLs of the entities of the first version.

<...>

More specifically, I think that at this point we only need versioning of the profiling data formats, not the API endpoints.

I'd add /v1 version on Pyroscope v1.0.0 release.

2. API Groups

By API Group I mean a set of handlers with a common URL prefix including the version.

I think that this is a premature measure that will complicate use of the pyroscope server API without any good for us and consumers (at this point at least).

1. Boundaries of a group can be very vague or an entity can be related to multiple groups. This is problem per se, because we will need to decide on this every time we encounter such a case.
2. If an entity from one group refers another entity from another group, versioning becomes non-trivial.
3. Every group has its own version and API consumers have to manage them. And I can't take a guess that group users won't overlap (in general case at least): we don't have this information yet.
4. In my experience, api version changes only when the whole app logic or API layer changes (as it should be). Therefore I assume, that in most cases API group versions will change at the same time, due to the same reasons, making this segregation useless.
5. There are common practices. I may be wrong here but vast majority of users expect either a /api/{version}/ prefix in the URL, or versioning via headers.

Maybe we have different visions of the API groups and I'm wrong, but I don't think API groups are helpful even in big projects.

3. Disabling endpoints

Can you elaborate on what problems disabling endpoints causes? I can see some benefits to disabling endpoints (e.g. on a pull-based deployment not disabling the ingestion endpoint is inconvenient, same for exposing the adhoc API while it's disabled, etc.)

We should disable features, not endpoints, I believe. API server should return a proper response with the corresponding code (e.g. 403) and a message, explaining that the feature is disabled. Otherwise, users may find it obscure: for example, as API consumer I want to know why GET /api/{collection}/{resource} returns 404: is it a typo in the URL, or it was disabled on the server side?

We don't need API groups to manage this.

4. Monolithic design

How we want to implement our API depends on what design we want to follow. If we want to have a monolithic design, that's a valid approach, but I do see value to have independent and unrelated components decoupled (e.g. having single api, model, etc. package that contains information about users, roles, apis, profiles, ingestion, adhoc, etc. is harder to maintain, in my opinion).

I fail to see how it's related. If this is matter of packages structure, then I have to say that using single package for multiple domains may not be the best approach for a big project, but in our case (and most cases) it works better: you don't need to import dozens of packages with colliding names and creating aliases for them, spend time on thinking whether this thing belongs to this domain or that, make some types and functions public only because of this, and so on.

Most of the API is centralized. A single controller took care of most of our API, now we have a new API package that contains a set of independent APIs that are coupled in the same package, instead of being modular.

I do believe that keeping all API handlers in one package (maybe with sub-packages, if needed) is more explicit, clear, and safe (read: better) than scattering the API logic among the repository. You can always register a subset of routes, if needed.

[abeaumont]

Great points, thanks for sharing! I think we agree on some of those points and disagree on some others, let's explore them a bit more :)

1. Versioning

More specifically, I think that at this point we only need versioning of the profiling data formats, not the API endpoints.

I'd add /v1 version on Pyroscope v1.0.0 release.

I'm fine with that. I agree that we don't need v1 from the very beginning, but I don't think it makes any harm either (especially if we end up with more than one version). But note that we already have some API endpoints with v1 and some without, we should, at least, normalize this, one way or another.

2. API Groups

I think the points you raise refer to the same granularity problem (which could be summarized with something like: if we have too many groups and they are coupled then it's worse for us and our users). I agree with this, and I don't think we should create groups for things that are related, they should be used for decoupled components. I gave some examples of what these components could be, let me go over them again and use them as an example. Let's three components/groups: adhoc, user management and profile data ingestion:

1. There's a clear boundary for all of them.
2. There should be no crossed-references between those because they are decoupled.
3. We don't know if users of the three APIs will overlap, but don't see why they would use all the three API groups at the same time within the same client.
4. I'm not sure why tying API changes to whole app changes is a good thing here. E.g. if we change the ingestion API because we realized that we made some mistakes in the original API that are penalizing us hard, why force adhoc or user management APIs to change? If we decide to remove adhoc completely, should we still keep the API available or force a whole new version for our whole API? I find it harder to come up with examples where the whole API needs to change at once. While it may make sense in some cases, I think the net result is that it makes it much harder to evolve the APIs when needed and it requires more coordination to bump the version numbers.
5. I agree with this, but I don't think it's a huge deal (REST APIs show a huge variance, after all, and the format of the version in the URI is rather minor in comparison).

3. Disabling endpoints

We should disable features, not endpoints, I believe. API server should return a proper response with the corresponding code (e.g. 403) and a message, explaining that the feature is disabled. Otherwise, users may find it obscure: for example, as API consumer I want to know why GET /api/{collection}/{resource} returns 404: is it a typo in the URL, or it was disabled on the server side?

We don't need API groups to manage this.

I'm fine with this. We can call them features instead of groups if that solves the granularity problem (that is, I'm not sure why API groups would have boundary problems but features would not, so maybe we are thinking about different granularity levels).

4. Monolithic design

I fail to see how it's related. If this is matter of packages structure,

It's related because a decoupled API design goes along with a decoupled API implementation more naturally, the same way a coupled API design goes along better with a coupled API implementation. Of course, there's no need to have them both together, but there's otherwise a mismatch that requires some extra work.

then I have to say that using single package for multiple domains may not be the best approach for a big project, but in our case (and most cases) it works better: you don't need to import dozens of packages with colliding names and creating aliases for them, spend time on thinking whether this thing belongs to this domain or that, make some types and functions public only because of this, and so on.

Maybe we have different ideas of what having decoupled components means, because it certainly doesn't require importing dozens of packages, or having a hard time deciding what parts belong to which component (e.g. the adhoc server is decoupled from the main controller and suffers from none of those problems). It certainly requires some work on the API (interfaces) of the components to isolate them (which is not mandatory, and can be built over time), which is a good thing, in my opinion. It also reduces significantly the cognitive load when working on some particular component, due to its isolation.

I do believe that keeping all API handlers in one package (maybe with sub-packages, if needed) is more explicit, clear, and safe (read: better) than scattering the API logic among the repository. You can always register a subset of routes, if needed.

Personally, I don't see much value in having it all in a single package, but I don't have a problem with that either, if we decide against API groups (I don't think subrouters or subpackages would make much of a difference in this approach, but I'm fine with those).

[abeaumont]

I wanted also to draw some conclusions but didn't want to mix them with discussion, so I'll put them in a separate comment here. I'd say that we have different views on the monolithic vs modular approach (both the API and implementation), and I think the rest of the points come from this difference. The approach is a trade-off, and Pyroscope is small enough to make it manageable in either case, so I'm fine with any of them, it's more important to be consistent with it, in my opinion. Based on the discussion so far, I think it makes sense to go with a more monolithic approach and leave decoupling aside, for now, maybe revisit it if things get less manageable. It'd be interesting to make these conclusions and the preferred approach more explicit, maybe in the architecture docs, so we are aligned here.

[kolesnikovae]

My point is not that API groups are harmful, but is that this is a premature measure. We're shouldn't make very bold assumptions in the very beginning.

Okay, so I think we already can make some decisions:

1. API Versioning

We use URL versioning, and put /api/v1 prefix in the routes, e.g:

• /api/v1/users/{id}
• /api/v1/adhoc/profile/{id}

Existing routes are still handled; to be removed in v1.0.0 release.

2. API Groups

We don't use API groups.

3. Disabling API handlers

API handlers of disabled features return 403 with the corresponding error message, if the feature is disabled.

For consistency, there should be a middleware that handles this based on the enabled features.

4. Packages layout

I find this topic pretty opinionated. I should say there are many options, I picked one that I like more based on my preferences, vision of the pyroscope future, and the current state.

Honestly, I can't agree that the api package exposes any issues with coupling or is related to monolithic design (same for service and model): I see nothing preventing us or making it harder using and maintaining these handlers and middlewares independently. The package can be used by multiple independent consumers to implement decoupled API architecture with no issues.

But I see your point that organizing packages by component instead of by layer may be beneficial (especially if we have very few components that can be easily isolated). If you feel strong about it, let's refactor. What I don't want to is to register http handlers in the component packages - the consumer may need only a subset of the HTTP API.

[abeaumont]

My point is not that API groups are harmful, but is that this is a premature measure. We're shouldn't make very bold assumptions in the very beginning.

Some design decisions are hard to change once they are made and I think this is one of those and that's why I opened the whole discussion, to begin with :) I have given some rationale about how this will make our design more modular, which should allow us to iterate components independently (and thus faster), and it will make it easier to build a scalable service out of it (yes, this is an assumption, but I don't think it's too bold). I have also shown explained how this can be achieved while keeping the approach simple enough. I'm fine with discarding the modular approach, but I don't think it's built on very bold assumptions :D.

Okay, so I think we already can make some decisions:

Yay :confetti_ball:

1. API Versioning

We use URL versioning, and put /api/v1 prefix in the routes, e.g:

:+1:

Existing routes are still handled; to be removed in v1.0.0 release.

:+1:

2. API Groups

We don't use API groups.

:+1:

3. Disabling API handlers

API handlers of disabled features return 403 with the corresponding error message, if the feature is disabled.

For consistency, there should be a middleware that handles this based on the enabled features.

Note that this means that we'll potentially have the business logic regarding what features are enabled/disabled in two different points (instead of one):

• The components that support the feature, which will need to know if it's enabled/disabled.
• A middleware that will have all the feature related logic available as well.

But this goes along the chosen design, so :+1:

4. Packages layout

I find this topic pretty opinionated. I should say there are many options, I picked one that I like more based on my preferences, vision of the pyroscope future, and the current state. [...] But I see your point that organizing packages by component instead of by layer may be beneficial (especially if we have very few components that can be easily isolated). If you feel strong about it, let's refactor. What I don't want to is to register http handlers in the component packages - the consumer may need only a subset of the HTTP API.

True that, the whole issue is mostly opinions on what's the best approach, in the API design, package design, etc. created because we already have 3 different API designs, which is the root problem. So I also picked the approach I consider best based on what we have. If we don't agree there, that's fine, we can keep a layered approach instead of a modular one.

Note that we need to refactor things in any case, as we have 3 different approaches already: first a controller with no layer separation, then a modular API for adhoc, finally a layered API for users. We should converge into one in any case. If that's the layered approach, we should refactor the original and adhoc APIs.

So, based on this I think we should:

• Move these decisions to some guidelines, https://pyroscope.io/docs/architecture/ looks a good location for that.
• Refactor the existing APIs to match the design decisions.
• Mark existing APIs that don't follow the preferred approach as deprecated.

[petethepig]

I was looking into different API endpoints we have and just wanted to document this here.

I generated all routes by adding this function to func (ctrl *Controller) serverMux() (http.Handler, error):

r.Walk(func(route *mux.Route, router *mux.Router, ancestors []*mux.Route) error {
  t, err := route.GetPathTemplate()
  if err != nil {
    logrus.WithError(err).Error("Failed to get route path template")
    return err
  }
  logrus.Debug(strings.Repeat("| ", len(ancestors)), t)
  return nil
})

And then I tried to structure them:

# FRONTEND
/                                            
/assets/                                     
/comparison                                  
/comparison-diff                             
/settings                                    
/settings/{page}                             
/settings/{page}/{subpage}                   
/adhoc-single                                
/adhoc-comparison                            
/adhoc-comparison-diff                       

# AUTH
/login                                       
/logout                                      
/signup                                      
/forbidden                                   

# MAIN FUNCTIONALITY
/render                                      
/render-diff                                 
/merge                                       
/ingest                                      
| /ingest                                    
/labels                                      
/label-values                                
/export                                      

# USERS / KEYS
/api                                         
| /api/users                                 
| | /api/users                               
| | /api/users                               
| | /api/users/{id:[0-9]+}                   
| | | /api/users/{id:[0-9]+}                 
| | | /api/users/{id:[0-9]+}                 
| | | /api/users/{id:[0-9]+}                 
| | | /api/users/{id:[0-9]+}/password        
| | | /api/users/{id:[0-9]+}/disable         
| | | /api/users/{id:[0-9]+}/enable          
| | | /api/users/{id:[0-9]+}/role            
| /api/user                                  
| | /api/user                                
| | /api/user                                
| | /api/user/password                       
| /api/keys                                  
| | /api/keys                                
| | /api/keys                                
| | /api/keys/{id:[0-9]+}                    
| | | /api/keys/{id:[0-9]+}                  


# HEALTH
/healthz           

# ADHOC
/api/adhoc                                   
| /api/adhoc/v1/profiles                     
| /api/adhoc/v1/profile/{id:[0-9a-f]+}       
| /api/adhoc/v1/diff/{left:[0-9a-f]+}/{right:[0-9a-f]+} 
| /api/adhoc/v1/upload/                      
| /api/adhoc/v1/upload-diff/                 



# DIAGNOSTICS endpoints
/metrics                                     
/exported-metrics                            
/build                                       
/targets                                   
/config                                        
/debug/storage/export/{db}                   
/debug/pprof/                                
/debug/pprof/cmdline                         
/debug/pprof/profile                         
/debug/pprof/symbol                          
/debug/pprof/trace                           
/debug/pprof/allocs                          
/debug/pprof/goroutine                       
/debug/pprof/heap                            
/debug/pprof/threadcreate                    
/debug/pprof/block                           
/debug/pprof/mutex