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

Normalize the dictionary order and values for Reporter Run Configs #3360

Closed

Conversation

LoopedBard3
Copy link
Member

@LoopedBard3 LoopedBard3 commented Sep 20, 2023

Normalize the dictionary order and values in the run configurations for the Reporter.cs generation. Also fixed some spacing. This will ensure that the RunConfiguration strings uploaded to the ADX will be consistent in ordering.

The OrdinalCaseInsensitive string comparer was chosen based on use of the default stringcomparer sort in other places and the values of the RunConfigs in our run information tables like "CompilationMode:tiered;iOSLlvmBuild:true;iOSStripSymbols:true;RunKind:ios_scenarios;RuntimeType:mono". Either Invariant or ordinal cultures with CaseInsensitivity would result in this for my testing, but using ordinal culture should ensure no potential machine to machine differences as the individual character code values are evaluated.

… Reporter.cs generation. Also fixed some spacing.
@LoopedBard3 LoopedBard3 added bug Something isn't working enhancement New feature or request labels Sep 20, 2023
@LoopedBard3 LoopedBard3 self-assigned this Sep 20, 2023
@LoopedBard3 LoopedBard3 marked this pull request as draft September 20, 2023 22:40
@LoopedBard3 LoopedBard3 marked this pull request as ready for review September 20, 2023 23:11
Copy link
Member

@DrewScoggins DrewScoggins left a comment

Choose a reason for hiding this comment

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

LGTM

@caaavik-msft
Copy link
Contributor

caaavik-msft commented Sep 20, 2023

Before we merge this in, I think it may be worth evaluating if we can remove any dependency we have on the sort order of the configuration keys here. If the only issue is in ADX reporting, I think it might be better to fix it in ADX's end by maybe adding a new column which concatenates all the config keys together in sorted order. This PR would cause configuration objects that were always unsorted to suddenly become sorted and will make the measurement history even more difficult to unify inside ADX.

@cincuranet
Copy link
Contributor

cincuranet commented Sep 21, 2023

For my own understanding, what's actually the reason to have it ordered? Is there some problem this is solving? Otherwise LGTM. @caaavik-msft updated me.

Copy link
Contributor

@caaavik-msft caaavik-msft left a comment

Choose a reason for hiding this comment

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

I'd like to block this PR until we've discussed this more since I think merging this will likely make things even more difficult when comparing runs from before this PR is merged in.

@DrewScoggins
Copy link
Member

One thing we should do here is get an answer on how many runs this will cause a break in, and when those runs have been happening. If only a small number of datapoints are going to be affected, I think that taking this change and just updating the old points should be worth it.

@LoopedBard3
Copy link
Member Author

One thing we should do here is get an answer on how many runs this will cause a break in, and when those runs have been happening. If only a small number of datapoints are going to be affected, I think that taking this change and just updating the old points should be worth it.

That makes sense, I will work on getting this information.

@LoopedBard3
Copy link
Member Author

One thing we should do here is get an answer on how many runs this will cause a break in, and when those runs have been happening. If only a small number of datapoints are going to be affected, I think that taking this change and just updating the old points should be worth it.

That makes sense, I will work on getting this information.

After some testing to get the information on the Run Configs that would be affected by this change or similar ones, over half of the run configs returned would be in a different order. This does not group already differently ordered runconfigs, such as the ones seen with the ios_scenarios. Instead, I switched to using the individual columns for each Run Config option for what I needed. While this may be worth revisiting, I am going to close for now as I don't need this change for the work I am doing.

@LoopedBard3 LoopedBard3 closed this Oct 3, 2023
@LoopedBard3 LoopedBard3 deleted the NormalizeRunConfigsInReporter branch August 5, 2024 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants