feat(profiler): implement ProfileType.UnmarshalText method#3881
Conversation
This supports configuration use cases where the profile type is parsed from text sources like environment variables or config file.
nsrip-dd
left a comment
nsrip-dd
left a comment
There was a problem hiding this comment.
LGTM. Stray thoughts that don't block this PR, putting here for posterity: it would be cool if we could automatically generate this for every exported ProfileType, but OTOH I don't anticipate adding new ones any time soon and it doesn't feel worth pulling in a third-party tool to do it. We could iterate over profileTypes, I suppose, but then we'd need to distinguish public and private types. Like, I wouldn't want to accept "trace" without some refactoring, or "goroutinewait" since we might delete it. What's in this PR is good for now.
Question for reviewers, should I implement a MarshalText method as well for completeness? These methods are related because UnmarshalText is expected to be able to unmarshal the output from MarshalText.
Personally I don't think that'll be necessary. We already have a String method that does basically the same thing, but we only use it internally. There's no way to get the enabled profile types back out of the profiler. We can add such a method later if somebody comes to us with a use-case.
2 participants
What does this PR do?
This change supports configuration use cases where the profile type is parsed from text sources like environment variables or config file, such that the parsed values can be passed to
profiler.WithProfileTypes.Question for reviewers, should I implement a
MarshalTextmethod as well for completeness? These methods are related becauseUnmarshalTextis expected to be able to unmarshal the output fromMarshalText.Motivation
Related to discussion: #3872
Reviewer's Checklist
./scripts/lint.shlocally.Unsure? Have a question? Request a review!