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

Remove usage of the stats endpoint to get origin package counts #719

Merged
merged 1 commit into from Oct 10, 2018

Conversation

Projects
None yet
4 participants
@elliott-davis
Member

elliott-davis commented Oct 10, 2018

Since the only place we were using the stats endpoint was to list the package counts for an origin I have completely removed it from the API.

This will also allow on-prem users to see their package counts for their origins.

Signed-off-by: Elliott Davis elliott@excellent.io

@thesentinels

This comment has been minimized.

Show comment
Hide comment
@thesentinels

thesentinels Oct 10, 2018

Contributor

Thanks for the pull request! Here is what will happen next:

  1. Your PR will be reviewed by the maintainers
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

Contributor

thesentinels commented Oct 10, 2018

Thanks for the pull request! Here is what will happen next:

  1. Your PR will be reviewed by the maintainers
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

@eeyun

This comment has been minimized.

Show comment
Hide comment
@eeyun

eeyun Oct 10, 2018

Contributor

Not that this is a blocker but I do have a couple of questions about this.
First, instead of grabbing package stats and publishing them to an API now you're effectively rendering just the package count somewhere in the UI?

Second, and possibly more importantly - are we removing this endpoint because it's not useful? Is it actually not useful, or is it just not being effectively utilized? I can see a reality where having an origin's stats endpoint could be super useful if the right pieces of data are being exposed. I'd hate to see this removed primarily for simplification of the API if there is future work that could be useful in here.

Contributor

eeyun commented Oct 10, 2018

Not that this is a blocker but I do have a couple of questions about this.
First, instead of grabbing package stats and publishing them to an API now you're effectively rendering just the package count somewhere in the UI?

Second, and possibly more importantly - are we removing this endpoint because it's not useful? Is it actually not useful, or is it just not being effectively utilized? I can see a reality where having an origin's stats endpoint could be super useful if the right pieces of data are being exposed. I'd hate to see this removed primarily for simplification of the API if there is future work that could be useful in here.

@elliott-davis

This comment has been minimized.

Show comment
Hide comment
@elliott-davis

elliott-davis Oct 10, 2018

Member

@eeyun to answer your first question, yes. Instead of sending back just the origin row from the origins table, I'm now sending back the row, plus the package count in the /users/origins endpoint.

This feature was added when we had an overall stats view in the UI. Upon looking at the metrics for page usage from users we decided to remove that view because it was so underutilized. We didn't remove the accompanying API because it was still in use to get the origin package counts for the "My Origins" view. The query that was removed was an extremely expensive call to the database and was called multiple times when a user clicked anywhere on an origin view. So, while this does reduce our overall API surface area it also reduces load on the database and enables on-prem users to see package totals (since the stats API was only exposed if you had jobsrv enabled)

Member

elliott-davis commented Oct 10, 2018

@eeyun to answer your first question, yes. Instead of sending back just the origin row from the origins table, I'm now sending back the row, plus the package count in the /users/origins endpoint.

This feature was added when we had an overall stats view in the UI. Upon looking at the metrics for page usage from users we decided to remove that view because it was so underutilized. We didn't remove the accompanying API because it was still in use to get the origin package counts for the "My Origins" view. The query that was removed was an extremely expensive call to the database and was called multiple times when a user clicked anywhere on an origin view. So, while this does reduce our overall API surface area it also reduces load on the database and enables on-prem users to see package totals (since the stats API was only exposed if you had jobsrv enabled)

#[derive(Debug, Serialize, Deserialize, QueryableByName)]
#[table_name = "origins_with_stats"]
pub struct OriginWithStats {

This comment has been minimized.

@chefsalim

chefsalim Oct 10, 2018

Member

I'm not sure I like the pattern of creating a different OriginWithStats struct... can we not modify the Origin struct?

@chefsalim

chefsalim Oct 10, 2018

Member

I'm not sure I like the pattern of creating a different OriginWithStats struct... can we not modify the Origin struct?

This comment has been minimized.

@elliott-davis

elliott-davis Oct 10, 2018

Member

The struct has to match the return type of the function unfortunately. I think we could fix this if we used regular diesel functions for querying and joining but by using stored procedures the compiler tries to type-check based on the returned "table" data. When we return origin rows in the future without a package count, we'll need the regular origin struct. I made an attempt at a struct like:

pub struct OriginsWithStats {
  pub origin: Origin,
  pub package_count: i64
} 

but I couldn't get the return types right from the sql function.

@elliott-davis

elliott-davis Oct 10, 2018

Member

The struct has to match the return type of the function unfortunately. I think we could fix this if we used regular diesel functions for querying and joining but by using stored procedures the compiler tries to type-check based on the returned "table" data. When we return origin rows in the future without a package count, we'll need the regular origin struct. I made an attempt at a struct like:

pub struct OriginsWithStats {
  pub origin: Origin,
  pub package_count: i64
} 

but I couldn't get the return types right from the sql function.

@chefsalim

This comment has been minimized.

Show comment
Hide comment
@chefsalim

chefsalim Oct 10, 2018

Member

Looks great, just one comment above

Member

chefsalim commented Oct 10, 2018

Looks great, just one comment above

Elliott Davis
Remove usage of the stats endpoint to get origin package counts
Since the only place we were using the stats endpoint was to list the pacakge counts for an origin I have completely remvoed it from the API.

This will also allow on-prem users to see their package counts for their origins.

Signed-off-by: Elliott Davis <elliott@excellent.io>
@eeyun

eeyun approved these changes Oct 10, 2018

@elliott-davis elliott-davis merged commit 2c0de69 into master Oct 10, 2018

2 checks passed

DCO This commit has a DCO Signed-off-by line
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@elliott-davis elliott-davis deleted the elliott/remove_stats branch Oct 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment