Skip to content

Conversation

@kotlarmilos
Copy link
Member

Description

This PR adds a linking‐type parameter to the configuration, allowing dashboards to compare static and dynamic linking.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new linking_type parameter throughout the performance job tooling so that dashboards can distinguish static vs. dynamic linking.

  • Added linking_type field and environment propagation in the Helix submission script
  • Exposed --linking-type CLI flag, default, and configuration in run_performance_job.py
  • Updated Azure pipeline template to pass the new flag when set

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
scripts/send_to_helix.py Added linking_type field and LinkingType environment var
scripts/run_performance_job.py Introduced CLI flag, default value, and config propagation
eng/pipelines/templates/run-performance-job.yml Conditionally include --linking-type in pipeline steps
Comments suppressed due to low confidence (3)

scripts/run_performance_job.py:92

  • Add validation for linking_type (e.g. allow only "Static" or "Dynamic") and raise an error on invalid values to prevent misconfiguration.
linking_type: str = "Dynamic"

scripts/send_to_helix.py:71

  • Update the argument parser help text or module docstring to describe the new linking_type parameter so users understand its purpose and accepted values.
linking_type: Optional[str] = None

scripts/run_performance_job.py:543

  • Add or update unit/integration tests to verify that the linking_type argument is parsed correctly and propagated into the configurations dictionary.
configurations["LinkingType"] = str(args.linking_type)

@kotlarmilos
Copy link
Member Author

The internal pipeline dotnet-runtime-perf succeeded and the dashboards are updated to include statically linked SOD measurements.

Copy link
Member

@LoopedBard3 LoopedBard3 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 good to me 👍. We will also need to update the autofiling configuration to include the LinkingType. As this is not its own column in the ADX, it will need to be pulled out in the query for android tests, but I think that doing this parse is fine for now as the number of tests that need this should not be too high. @caaavik-msft any thoughts on whether we should add this as it's own column for ADX?

@kotlarmilos
Copy link
Member Author

We will also need to update the autofiling configuration to include the LinkingType. As this is not its own column in the ADX, it will need to be pulled out in the query for android tests, but I think that doing this parse is fine for now as the number of tests that need this should not be too high.

Correct. I updated the dashboard to filter based on this value.

@LoopedBard3
Copy link
Member

Cool. I think this is good to merge as the ADX column question can be answered next week and adding this in minimizes any mislabeled data.

@kotlarmilos kotlarmilos enabled auto-merge (squash) June 5, 2025 17:18
@LoopedBard3 LoopedBard3 disabled auto-merge June 5, 2025 17:46
@LoopedBard3 LoopedBard3 merged commit 028daae into main Jun 5, 2025
76 of 80 checks passed
@LoopedBard3 LoopedBard3 deleted the improvement/add-linking-type-parameter branch June 5, 2025 17:47
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.

3 participants