Skip to content

Log experiments run via ExP in the output panel#12742

Merged
kimadeline merged 9 commits into
microsoft:masterfrom
kimadeline:12656-exp-log
Jul 7, 2020
Merged

Log experiments run via ExP in the output panel#12742
kimadeline merged 9 commits into
microsoft:masterfrom
kimadeline:12656-exp-log

Conversation

@kimadeline
Copy link
Copy Markdown

@kimadeline kimadeline commented Jul 3, 2020

For #12656

🕯️ We need to filter the experiments we log since the ExP platform is also used by other teams. After talking with @luabud we agreed to prefix all future experiment names with python (will update the documentation). The start page experiment doesn't follow this convention, but it should be phased out soon so it's not a problem.

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated.
  • Test plan is updated as appropriate.
  • package-lock.json has been regenerated by running npm install (if dependencies have changed).
  • The wiki is updated with any design decisions/details.

@kimadeline kimadeline marked this pull request as ready for review July 6, 2020 23:26
}

private logExperiments() {
const experiments = this.globalState.get<{ features: string[] }>(EXP_MEMENTO_KEY, { features: [] });
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does this return all experiments the user belongs to? Or all experiments running on behalf of the Python extension.

We should be logging the prior one, right? The news entry and naming is making it seem like we're logging all the experiments, and not just the one the user belongs to.

Copy link
Copy Markdown
Author

@kimadeline kimadeline Jul 7, 2020

Choose a reason for hiding this comment

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

If the user doesn't belong to an experiment it won't be in storage, so by default it returns all experiments and control groups the user belongs to. Then because we don't want to log experiments that are not run by the Python extension (agreed on with Luciana) we filter them out.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I don't think the news entry nor the naming are ambiguous (we're logging experiments inside the Python extension, why would we log experiments that are not from the Python extension), but I updated the issue content provide an answer to your question.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This isn't a public API (accessing the stored experiments from the global storage), shouldn't we ask for a public API instead.
I.e. the storage key isn't public.

Copy link
Copy Markdown
Author

@kimadeline kimadeline Jul 7, 2020

Choose a reason for hiding this comment

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

We asked, and they said they were working on it. If I remember correctly other teams also asked for this feature, so hopefully there will be an "official" way to do it soon. In the meantime, they are the ones who provided us with this workaround.

Copy link
Copy Markdown

@karrtikr karrtikr Jul 7, 2020

Choose a reason for hiding this comment

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

we're logging experiments inside the Python extension, why would we log experiments that are not from the Python extension

What I was trying to say is, by looking at the news entry which says
Log experiments run via ExP in the output panel
It seems we're logging all experiments registered in the Python extension - whether the user is in it or not.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sure, I can amend it to be "Log ExP experiments the user belongs to in the output panel.".

Copy link
Copy Markdown

@karrtikr karrtikr Jul 7, 2020

Choose a reason for hiding this comment

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

Great, I also recommend adding a doc comment in the method logExperiments(), which says it logs the experiment the user belongs to.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The behaviour is similar as the existing experiment manager, which already only logs the experiments the user belongs to. Instead I'll clarify the existing comment to say that we filter out experiments that are not run by the Python extension.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jul 7, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@kimadeline kimadeline merged commit a2d6ac7 into microsoft:master Jul 7, 2020
@kimadeline kimadeline deleted the 12656-exp-log branch July 7, 2020 19:18
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.

4 participants