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

[fix] logging behavior for query hashes #759

Merged
merged 3 commits into from Sep 2, 2021

Conversation

criess
Copy link
Contributor

@criess criess commented Sep 2, 2021

  • we must log only the query when testing for the query as hash
  • see modified behavior inside Excon::Instrumentors::LoggingInstrumentor
  • added very simple test

We found out when our logs got flooded with excon client options information, so I decided to fix it right away for upstream projects.

Let me know when you need changes, other ideas on this PR. Anyways patch is very minor.

Thanks, CR

  * we must log only the query when testing for the query as hash
  * see modified behavior inside Excon::Instrumentors::LoggingInstrumentor
  * added very simple test
Copy link
Contributor

@geemus geemus left a comment

Choose a reason for hiding this comment

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

Looks good overall. This was almost assuredly the original intention of the code, it's interesting that it was overlooked for so long. Thanks for working on and sharing a fix!

@@ -26,6 +26,33 @@
]
end

tests("connection logger with query as hash").returns(true) do
Excon.stub({:method => :get}, {body: 'body'}, status: 200})
Copy link
Contributor

Choose a reason for hiding this comment

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

It appears there is an extra } after the status portion here that is causing a syntax error in tests. Could you fix it please?

@criess criess requested a review from geemus September 2, 2021 18:11
Copy link
Contributor

@geemus geemus left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks again for the report and then working through the troublesome test bits!

@geemus geemus merged commit 973e581 into excon:master Sep 2, 2021
@criess criess deleted the logging-instrumentor-queries-as-hash branch September 3, 2021 07:23
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.

None yet

2 participants