-
Notifications
You must be signed in to change notification settings - Fork 629
Stabilize observability #4563
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
base: master
Are you sure you want to change the base?
Stabilize observability #4563
Conversation
c224f58 to
8d25f1c
Compare
a1bc62b to
d4a3459
Compare
8630d99 to
4eb82da
Compare
Zac-HD
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(incomplete thoughts)
Zac-HD
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more notes after our in-person conversation; settings issue coming soon.
d9b6232 to
0d8896b
Compare
|
This is ready for review 👍 (it's a big one!) |
a793f08 to
4fe2f91
Compare
Zac-HD
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(oops, apparently my weeks-ago review didn't upload properly 😓(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine to merge as-is, but I'd like to split out a how-to guide and trim down the reference considerably as a followup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is probably big enough that we should pull out a short blog post on observability!
(which will also allow us to cut this changelog down to a more reasonable size)
| old_value = self.settings.observability | ||
| self.settings._observability = ( | ||
| ObservabilityConfig(callbacks=[callback]) | ||
| if old_value is None | ||
| else dataclasses.replace( | ||
| old_value, callbacks=old_value.callbacks + (callback,) | ||
| ) | ||
| ) | ||
| try: | ||
| yield | ||
| finally: | ||
| self.settings._observability = old_value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels off somehow.
- wrong mechanism for setting a callback maybe
- also, I'd like to make backends give us an ObservabilityConfig rather than a callback alone
will think further on it though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, I'd like to make backends give us an ObservabilityConfig rather than a callback alone
agreed, was punting this due to the size of this PR
|
Given the size and age, pulling bits out for easier merging is a great strategy. I think we're fairly close here, but... |
Zac-HD
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is in great shape, but removing the experimental/provisional status means that I want to tune a few last things before we merge:
-
PrimitiveProvidershould have anObservabilityConfigas an attribute, not the current callback
and there are some followups for later
- blog post announcing it + listing some new research ideas etc
- think about better data formats for coverage info
- hypofuzz integration (simplifying coverage collection)
- better multi-thread
_deliver_to_fileperformance via a cheap deque + lock
and then I dropped this for a few days, leaving just the following diff: Liam-DeVoe/hypothesis@stabilize-observability...Zac-HD:hypothesis:zac/update-observability
I'll probably get back to it next weekend, but it might be worth pulling out some smaller stuff that can be merged without taking the time to think carefully about the whole API change. (deliver-to-file, maybe the PrimitiveProvider.observability thing too as an internal/unstable interface?)
Closes #4387.
@settings(observability=...)takesTrue,False, orObservabilitySettings. If not set, inherits fromHYPOTHESIS_OBSERVABILITY, which is either true or false (noObservabilitySettingsparsing/control).HYPOTHESIS_EXPERIMENTAL_OBSERVABILITY_NOCOVERremoved with no deprecation periodHYPOTHESIS_EXPERIMENTAL_OBSERVABILITYandHYPOTHESIS_EXPERIMENTAL_OBSERVABILITY_CHOICESkept for backwards-compat, but will be removed eventuallyobservability.OBSERVABILITY_COLLECT_COVERAGEandobservability.OBSERVABILITY_CHOICESreplaced withobservability.OBSERVABILITY_SETTINGSFor a future PR: with observability callbacks, there's no way to control
ObservabilitySettings. You don't want to set@settings(observability=ObservabilitySettings(...))on the test, because this would write to.hypothesis/observed, and you just want the observations in-memory.You can do this by monkeypatching
observability.OBSERVABILITY_SETTINGS. (indeed, hypofuzz does this right now). I'd like to get rid of that var, and addadd_observability_callback(options: ObservabilitySettings). But semantic questions like "what if two callbacks where only one setscoverage=True" have to be answered. I do think this can be done, I just don't want to do it right now. So I've left the var in, but undocumented.old description
@settings(observability=...)takes three possible values:observability=Trueenables observabilityobservability=Falsedisables observabilityobservability=[option1, option2], likeobservability=["coverage"], enables observability, and also enables whatever options are specified@settings(observability=...)is not set, inherit fromHYPOTHESIS_OBSERVABILITYenvvarHYPOTHESIS_EXPERIMENTAL_OBSERVABILITY_NOCOVERremoved with no deprecation periodHYPOTHESIS_EXPERIMENTAL_OBSERVABILITYandHYPOTHESIS_EXPERIMENTAL_OBSERVABILITY_CHOICESkept for backwards-compat because it was easy, but are now-undocumented and will be removed at some pointI'd like to keep a very clear separation between
@settings(observability=...), andadd_observability_callback. IMO the former should always write to/be related to.hypothesis/observed, and the latter should never be (ie it's sent in-memory).This brings up a non-trivial issue, probably for a future PR: when adding observability callbacks, there's no way to control the
coverageandchoicesoptions. You don't want to set@settings(observability=["choices"])on the test/profile, because this would write to.hypothesis/observed, and you just want the observations in-memory. Previously this was done withobservability.OBSERVABILITY_COLLECT_COVERAGEandobservability.OBSERVABILITY_CHOICES. I'd like to get rid of those vars eventually, and addadd_observability_callback(options=["coverage"]). But semantic questions like "what if two callbacks where only one sets options=["coverage"] have to be answered. I do think this can be done, I just don't want to do it right now. So I've left those vars in, though now un-documented.OBSERVABILITY_SETTINGSI'd also love to write a hypothesis.works blog post about this, at some point..
[cc @hgoldstein95]