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

Handle metric names from legacy spark #1052

Merged
merged 1 commit into from
May 31, 2024

Conversation

amahussein
Copy link
Collaborator

Signed-off-by: Ahmed Hussein (amahussein) [email protected]

Fixes #1042

Running the tool on a legacy spark eventlog (2.x) caused problems because the SQL metric names are not the same

Changes

This PR adds a map between <legacy-metric-names, new-metric-names>

  • The map is looked-up in the following parts:
    • During the construction of an AccumulableInfo: this guarantees that all metrics created in a TaskEnd/Stage are mapped to the valid names
    • During the contruction of the SQLplanGraph: this guarantees that node metrics are mapped to the most recent labels
  • I added an extra match-pattern to avoid this failure from happening in the future. When the mtric-name is missing, there will be a warning message that it is not handled correctly.
  • The process to do the fix was painful because I had to look into all spark versions to spot any metric-name change. t is possible that I did not catch them all, but at least we can append more mapping in the future.

I had to change UT because the changes actually fixed a hidden bug in our code.

Signed-off-by: Ahmed Hussein (amahussein) <[email protected]>

Fixes NVIDIA#1042
@amahussein amahussein added bug Something isn't working core_tools Scope the core module (scala) labels May 30, 2024
@amahussein amahussein self-assigned this May 30, 2024
Copy link
Collaborator

@nartal1 nartal1 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @amahussein ! That's a long list of legacy metrics name :)

Copy link
Collaborator

@parthosa parthosa left a comment

Choose a reason for hiding this comment

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

Thanks @amahussein for this. In the E2E testing doc, I will add an entry for integration testing on legacy spark event logs.

@amahussein amahussein merged commit 1f4d888 into NVIDIA:dev May 31, 2024
16 checks passed
@amahussein amahussein deleted the spark-rapids-tools-1042 branch May 31, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core_tools Scope the core module (scala)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] MatchError in getDataSourceInfo
3 participants