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

Add potential multi-dataset logger #4920

Merged
merged 5 commits into from
Oct 5, 2023

Conversation

luismulinari
Copy link
Contributor

@luismulinari luismulinari commented Oct 4, 2023

Description

This PR adds a log entry containing the request information and backtrace when a potential multi-dataset query is detected.

Changelog Description

N/A

Pre-review checklist

  • This change works and has been tested locally (or has an appropriate fallback).
  • This change works and has been tested on a Go sandbox.
  • This change has relevant unit tests (if applicable).
  • This change uses a rollout method to ease with deployment (if applicable - especially for large scale actions that require writes).
  • This change has relevant documentation additions / updates (if applicable).
  • I've created a changelog description that aligns with the provided examples.

Pre-deploy checklist

  • VIP staff: Ensure any alerts added/updated conform to internal standards (see internal documentation).

Steps to Test

  1. Check out PR.
  2. Perform a SQL query across multiple datasets
  3. Observe logstash logs

@luismulinari luismulinari requested a review from a team as a code owner October 4, 2023 20:30
@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Merging #4920 (f75d518) into develop (280f1c3) will increase coverage by 0.10%.
The diff coverage is 94.44%.

@@              Coverage Diff              @@
##             develop    #4920      +/-   ##
=============================================
+ Coverage      29.12%   29.22%   +0.10%     
- Complexity      4744     4748       +4     
=============================================
  Files            278      278              
  Lines          20887    20905      +18     
=============================================
+ Hits            6083     6110      +27     
+ Misses         14804    14795       -9     
Files Coverage Δ
...lass-potential-multi-dataset-queries-collector.php 90.19% <94.44%> (+2.31%) ⬆️

... and 7 files with indirect coverage changes

Copy link
Member

@mjangda mjangda left a comment

Choose a reason for hiding this comment

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

Left some notes.

@luismulinari luismulinari requested a review from mjangda October 5, 2023 14:17
Copy link
Member

@mjangda mjangda left a comment

Choose a reason for hiding this comment

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

😎

@luismulinari luismulinari merged commit a6adfed into develop Oct 5, 2023
@luismulinari luismulinari deleted the add/multi-dataset-query-logger branch October 5, 2023 17:59
andrea-sdl pushed a commit that referenced this pull request Oct 19, 2023
* Add potential multi dataset logger

* Sanitize fields

* Use esc_url_raw since sanitize_url is deprecated

* Update prometheus-collectors/class-potential-multi-dataset-queries-collector.php

Co-authored-by: Mohammad Jangda <[email protected]>

* Check if log2logstash is defined before calling it

---------

Co-authored-by: Mohammad Jangda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants