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

SQL Datasources: Prevent Call Stack Overflows with Large Numbers of Values for Variable #64937

Merged
merged 4 commits into from Mar 22, 2023

Conversation

codeincarnate
Copy link
Collaborator

This PR prevents call stack overflows in SQL datasources. This can happen when getting variable values because of a spread operator (which has a map used inside) causing a large number of function calls to be added to the stack. This PR removes the spread operator and instead adds variable values directly from the call to map.

@codeincarnate codeincarnate added the area/backend/db/sql Back-end SQL database logic label Mar 17, 2023
@codeincarnate codeincarnate added this to the 9.5.0 milestone Mar 17, 2023
@codeincarnate codeincarnate requested a review from a team as a code owner March 17, 2023 05:52
@codeincarnate codeincarnate requested review from academo, mdvictor and baldm0mma and removed request for a team March 17, 2023 05:52
@codeincarnate codeincarnate changed the title SQL Datasources: Prevent Call Stack Overflows with Large Numbers of Values SQL Datasources: Prevent Call Stack Overflows with Large Numbers of Values for Variable Mar 17, 2023
@codeincarnate codeincarnate added the backport v9.4.x Mark PR for automatic backport to v9.4.x label Mar 17, 2023
@grafanabot
Copy link
Contributor

Hello @codeincarnate!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

Copy link
Contributor

@mdvictor mdvictor left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@baldm0mma baldm0mma left a comment

Choose a reason for hiding this comment

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

Good catch! LGTM.

text: v,
}))
);
frame.fields.flatMap((f) => f.values.toArray()).map((v) => values.push({ text: v }));
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a question, is there a reason you're using a map() here instead of a forEach() or just for of loop? Since you're not returning anything, would a forEach() make more sense?

Copy link
Member

Choose a reason for hiding this comment

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

@baldm0mma I refactored this to simple for of loops. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally think it's more readable, and therefore more maintainable; big fan!

@zoltanbedi zoltanbedi modified the milestones: 9.5.0, 9.4.x Mar 22, 2023
Copy link
Contributor

@mdvictor mdvictor left a comment

Choose a reason for hiding this comment

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

LGTM!

describe('transformMetricFindResponse function', () => {
it('should handle big arrays', () => {
const responseParser = new ResponseParser();
const stringValues = new Array(150000);
Copy link
Contributor

@baldm0mma baldm0mma Mar 22, 2023

Choose a reason for hiding this comment

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

Great idea on adding this test! Do we need to import fill() from lodash here? I believe we can accomplish the same thing with the native JS fill() function -> new Array(150_000).fill('a');

Copy link
Member

Choose a reason for hiding this comment

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

TIL 😱

@baldm0mma
Copy link
Contributor

LGTM!

@zoltanbedi zoltanbedi merged commit bf687ff into main Mar 22, 2023
12 checks passed
@zoltanbedi zoltanbedi deleted the codeincarnate/fix-sql-callstack branch March 22, 2023 13:50
grafanabot pushed a commit that referenced this pull request Mar 22, 2023
…alues for Variable (#64937)

* Push values with every map call to avoid hitting the maximum call stack size.

* Add test and refactor to for of

* Use native fill instead of lodash

---------

Co-authored-by: Zoltán Bedi <zoltan.bedi@gmail.com>
(cherry picked from commit bf687ff)
zoltanbedi pushed a commit that referenced this pull request Mar 22, 2023
…bers of Values for Variable (#65182)

SQL Datasources: Prevent Call Stack Overflows with Large Numbers of Values for Variable (#64937)

* Push values with every map call to avoid hitting the maximum call stack size.

* Add test and refactor to for of

* Use native fill instead of lodash

---------

Co-authored-by: Zoltán Bedi <zoltan.bedi@gmail.com>
(cherry picked from commit bf687ff)

Co-authored-by: Kyle Cunningham <codeincarnate@users.noreply.github.com>
gotjosh pushed a commit that referenced this pull request Mar 27, 2023
…alues for Variable (#64937)

* Push values with every map call to avoid hitting the maximum call stack size.

* Add test and refactor to for of

* Use native fill instead of lodash

---------

Co-authored-by: Zoltán Bedi <zoltan.bedi@gmail.com>
@zerok zerok modified the milestones: 9.4.x, 9.4.8, 9.5.0 Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/backend/db/sql Back-end SQL database logic area/frontend backport v9.4.x Mark PR for automatic backport to v9.4.x type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants