Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

Better serve logging #50

Open
tamj0rd2 opened this issue Apr 19, 2020 · 6 comments
Open

Better serve logging #50

tamj0rd2 opened this issue Apr 19, 2020 · 6 comments
Labels
enhancement New feature or request

Comments

@tamj0rd2
Copy link
Owner

It used to spit out quite a lot. Not sure what the state of it is nowadays.

@tamj0rd2 tamj0rd2 added the verify Functionality needs double checking label Apr 19, 2020
@tamj0rd2 tamj0rd2 added bug Something isn't working and removed verify Functionality needs double checking labels Apr 26, 2020
@tamj0rd2
Copy link
Owner Author

tamj0rd2 commented Apr 26, 2020

There's some pretty bad logging duplication/weirdness going on.

Reproduction:

Have a single config with these fields:

  • endpoint /api/resource?sort=asc
  • endpoint /api/resource?sort=desc
  • serve endpoint /api/resource

Make a request and wait for almost a dozen confusing logs. That's because 3 endpoints get registered with express, all with the same /api/resource endpoint.

There needs to be some kind of endpoint grouping. Like, maybe all of this should fit into one snug config where the endpoint is /api/resource and that config could have multiple query configurations.

Possible ServerConfig:

// ...
request: {
  endpoint: '/api/resource',
  queryConfigurations: [{ sort: 'asc' }, { sort: 'desc' }, {}],
},
// ...

That'd work pretty well seeing that for each individual raw config, the only thing that can possibly differ are the base endpoint and the query. Everything else stays exactly the same.

Here's another case I can write a test for:

Have a single config with these fields:

  • endpoint /api/resource/1?debug=true
  • endpoint /api/resource/2?debug=false
  • serve endpoint /api/resource/id

Maybe in serve mode, the endpoints should just be ignored completely if serveEndpoint is given? Not sure. I think I need to test the above case and actually check if express will try to trigger that route 3 times even though it should already be satisfied once.

@tamj0rd2
Copy link
Owner Author

tamj0rd2 commented Apr 26, 2020

Oh so the problem is that I actually just missed a return statement after the query check validation. Whoops! That's why it seemed like took many routes were triggering. That, and I'm sleepy

tamj0rd2 added a commit that referenced this issue Apr 26, 2020
I forgot to return after doing the query validation, so even if the query was found to be invalid it
would still continue trying to serve the request using the same handler

re #50
tamj0rd2 added a commit that referenced this issue Apr 26, 2020
I forgot to return after doing the query validation, so even if the query was found to be invalid it
would still continue trying to serve the request using the same handler

re #50
tamj0rd2 added a commit that referenced this issue Apr 26, 2020
I forgot to return after doing the query validation, so even if the query was found to be invalid it
would still continue trying to serve the request using the same handler

re #50
@tamj0rd2
Copy link
Owner Author

tamj0rd2 commented Apr 26, 2020

  • Do the grouping thing mentioned above because it's actually a pretty good idea and will help get rid of duplicate messages

These warnings aren't particularly helpful if there are multiple query configurations:

2020-04-26 23:42:35 - warn: An endpoint for /api/route exists but the query params did not match the configuration
2020-04-26 23:42:35 - warn: An endpoint for /api/route exists but the query params did not match the configuration

  • It either shouldn't be logged twice, or there should be a bit more detail about why the query doesn't match, and which configuration it's not matching.

Possible message: warn: An endpoint for /api/route exists but the query params did not match any of the given configurations

  • Also, error/info logs take up like 5-6 lines sometimes. They can probably just made to take a single line rather than being so pretty. Or maybe there should be an option to control that.

@tamj0rd2 tamj0rd2 added enhancement New feature or request and removed bug Something isn't working labels Apr 26, 2020
@tamj0rd2
Copy link
Owner Author

tamj0rd2 commented May 7, 2020

  • The "registering X from Y" logs can be way too much. They should probably become debug log level only

@tamj0rd2
Copy link
Owner Author

tamj0rd2 commented May 8, 2020

  • Once the server has started, handling of request type validation could be better. Currently it's this:
serverLogger.warn(
  `An endpoint for ${req.path} exists but the request body did not match the type`,
  { problems: validationResult },
)

@tamj0rd2
Copy link
Owner Author

  • It'd be extremely useful if this actually told you why a query or the headers don't match.

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

No branches or pull requests

1 participant