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

chore(gateway): debug logging for the http requests #8518

Merged
merged 8 commits into from
Feb 15, 2022

Conversation

manute
Copy link
Contributor

@manute manute commented Oct 17, 2021

This PR tries to solve this issue #7873

Please let me know if my approach is ok with the code and the issue.

Thanks!

@welcome

This comment has been minimized.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable.

core/corehttp/gateway_handler.go Outdated Show resolved Hide resolved
@BigLep BigLep requested a review from lidel October 29, 2021 06:47
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @manute, this is useful!

Noticed some cosmetic nits to address before we can merge this. Listed them below.

TODO

  • document ipfs log level core/server debug in /docs/gateway.md
  • those error (type nil) messages are noisy, are really hard to parse.
    To illustrate, curl -H "Host: ipfs.io" 127.0.0.1:8080 produces:
    2021-10-29T22:08:13.646+0200	DEBUG	core/server	corehttp/gateway_handler.go:202	http request received	{"from": "/ipns/ipfs.io/"}
    2021-10-29T22:08:13.647+0200	DEBUG	core/server	corehttp/gateway_handler.go:390	error (type nil) getting file tree for index.html, serve the file	{"from": "/ipns/ipfs.io/", "path": "/ipfs/QmPgJLQHLi5fNLTC2DJABJTa9T7dHy55TvKzatrvQS54B2/index.html"}
    
    A simple serving index.html file would be easier to reason about here.
  • redirect from localhost:8080/ipfs/bafybeic7etg7wgzgyhvkyl62iaa7klhxlatjw5uowccvzxxa72aeo5eqsu to the subdomain is not logged at all
  • subdomain requests are logged with duplicated CID (once in host, once in path). Opening http://bafybeic7etg7wgzgyhvkyl62iaa7klhxlatjw5uowccvzxxa72aeo5eqsu.ipfs.localhost:8080/
    produces:
    2021-10-29T21:57:17.142+0200	DEBUG	core/server	corehttp/gateway_handler.go:202	http request received	{"from": "http://bafybeic7etg7wgzgyhvkyl62iaa7klhxlatjw5uowccvzxxa72aeo5eqsu.ipfs.localhost:8080/ipfs/bafybeic7etg7wgzgyhvkyl62iaa7klhxlatjw5uowccvzxxa72aeo5eqsu/"}
    

We will have limited availability during the next week and a half, so if you have time to address the above during that time, would make it easier for us to include this in v0.11.0-rc1

@manute
Copy link
Contributor Author

manute commented Nov 1, 2021

Hey @lidel,

Thanks for the review, I have addressed some changes based on that.
A few questions:

redirect from localhost:8080/ipfs/bafybeic7etg7wgzgyhvkyl62iaa7klhxlatjw5uowccvzxxa72aeo5eqsu to the subdomain is not logged at all

Where I can find those subdomain redirects?

subdomain requests are logged with duplicated CID (once in host, once in path)

Do you have any ideas to remove that duplication? Shall I do that before this line? And do you have any utility library that parses the path w/o that duplication?

@lanzafame
Copy link
Contributor

@manute @Stebalien It may be even better to create a separate logger from the core/server logger that logs the requests, i.e. an access log similar to nginx. That way one can just monitor gateway requests without getting internal server errors. And having it as a separate logger will make it easier to filter out with tools such as loki or elasticsearch.

@Stebalien
Copy link
Member

@manute @Stebalien It may be even better to create a separate logger from the core/server logger that logs the requests, i.e. an access log similar to nginx. That way one can just monitor gateway requests without getting internal server errors. And having it as a separate logger will make it easier to filter out with tools such as loki or elasticsearch.

That's a good point. Logging to gateway/requests, or something like that, would be nice.

Where I can find those subdomain redirects?

https://github.com/ipfs/go-ipfs/blob/52a747763f6c4e85b33ca051cda9cc4b75c815f9/core/corehttp/hostname.go#L97

Ish. It's an interesting redirect because we return the new path, but also process the request.

Maybe we should just log response information in https://github.com/ipfs/go-ipfs/blob/52a747763f6c4e85b33ca051cda9cc4b75c815f9/core/corehttp/gateway_handler.go#L83? That's probably the best approach and that might just cover everything.

Do you have any ideas to remove that duplication? Shall I do that before this line? And do you have any utility library that parses the path w/o that duplication?

Hm. Maybe use the RequestURI instead? That's the URL that the user requested, before modifications, cleaning, etc.

@manute
Copy link
Contributor Author

manute commented Nov 3, 2021

Hey @Stebalien @lidel ,
I think I've addressed all your feedback. Please, let me know what do you think.

@lanzafame re:

@manute @Stebalien It may be even better to create a separate logger from the core/server logger that logs the requests, i.e. an access log similar to nginx. That way one can just monitor gateway requests without getting internal server errors. And having it as a separate logger will make it easier to filter out with tools such as loki or elasticsearch.

I think it's a good idea although I'd prefer to do that in another PR along with another issue, as it could have other implications.

Thanks!

@manute
Copy link
Contributor Author

manute commented Nov 15, 2021

@Stebalien @lidel Hey , does it need more changes or is it all fine?

@hyperknot
Copy link

Can this be merged in? Otherwise I see no reason to debug gateway configuration issues.

@guseggert guseggert merged commit edb32ac into ipfs:master Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants