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

refactor(log): store log in context so it's automatically propagated #1356

Merged
merged 6 commits into from
Mar 19, 2024

Conversation

ThinkChaos
Copy link
Collaborator

@ThinkChaos ThinkChaos commented Jan 28, 2024

As I had mentioned in #1264, this stores the logger in the Context we pass around during resolving. This helps better propagate log fields.
The most user visible impact is prefix now contains more resolvers in the chain.
There's also a new req_id field that's a unique ID per request which will making following logs during debug easier (related to #1307).

I also changed util.LogOnError to get the log from a context, and removed model.Request.Log.

Copy link

codecov bot commented Jan 28, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 94.24%. Comparing base (fe84ab8) to head (7d8f30f).
Report is 36 commits behind head on main.

Files Patch % Lines
server/server_endpoints.go 60.00% 5 Missing and 1 partial ⚠️
resolver/caching_resolver.go 66.66% 5 Missing ⚠️
resolver/parallel_best_resolver.go 60.00% 3 Missing and 1 partial ⚠️
resolver/query_logging_resolver.go 40.00% 3 Missing ⚠️
server/server.go 94.91% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1356      +/-   ##
==========================================
+ Coverage   93.88%   94.24%   +0.35%     
==========================================
  Files          78       78              
  Lines        6361     4933    -1428     
==========================================
- Hits         5972     4649    -1323     
+ Misses        300      194     -106     
- Partials       89       90       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kwitsch
Copy link
Collaborator

kwitsch commented Jan 29, 2024

This looks nice ♥️

Copy link
Collaborator

@kwitsch kwitsch left a comment

Choose a reason for hiding this comment

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

Just one question out of curiosity

resolver/ecs_resolver.go Show resolved Hide resolved
Copy link
Collaborator

@kwitsch kwitsch left a comment

Choose a reason for hiding this comment

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

👍

@kwitsch kwitsch mentioned this pull request Jan 6, 2024
11 tasks
Copy link
Contributor

@BenMcH BenMcH left a comment

Choose a reason for hiding this comment

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

This looks like a positive change! 🎉 Just had one (unrelated) question

resolver/caching_resolver.go Show resolved Hide resolved
RFC 8484 Section 4.2.1:
> A successful HTTP response with a 2xx status code (see
> Section 6.3 of RFC7231) is used for any valid DNS response,
> regardless of the DNS response code.  For example, a successful 2xx
> HTTP status code is used even with a DNS message whose DNS response
> code indicates failure, such as SERVFAIL or NXDOMAIN.
https://www.rfc-editor.org/rfc/rfc8484#section-4.2.1
@ThinkChaos ThinkChaos merged commit 73e5d6a into 0xERR0R:main Mar 19, 2024
10 of 11 checks passed
@ThinkChaos ThinkChaos deleted the refactor/log-ctx branch March 19, 2024 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants