Add guardrails to Profiler class

[debadutta98]

Hi @wbt ,

This issue Closes #2091

Work

• Force the Profiler class to only accept an object which is an instance of Logger
• change test case in test\unit\winston\profiler.test.js failed due to above changes

Thank you waiting for your response

[wbt]

Can you say a few words about backward compatibility?

[debadutta98]

Can you say a few words about backward compatibility?

Thanks, @wbt for asking me this question

Sorry, this term is totally new to me. but do some research on it I think this term is something that is related to testing. if I am wrong then please correct me

I saw that CI testing is failing because of linting issues which I already fix that

[wbt]

The question about backward compatibility is, in more detail: Is it possible that existing non-Winston code which uses the existing Winston code to do something someone plausibly finds useful will not work after this is merged? At first glance, the answer appears affirmative: if you pass a non-logger argument to the Profiler constructor, it will throw an error where it might not have before. What was the prior behavior? Is it possible that it was still doing something useful?

If so, we should note the breaking change in a "breaking" increase in the semantic versioning.

@maverick1872 might also have some inputs and be in a better position to review/merge.

[maverick1872]

I'll be honest I'm not super familiar with the Profiler implementation myself but do remember desiring this to be hardened. I hold the same concerns that @wbt does. This would likely result in a breaking change and necessitate a major bump. Additionally before I'd accept this I'd like to see test cases added to ensure that the guardrail functions as expected as well.

[wbt]

@debadutta98 I don't read either of the comments above as rejecting or requiring closure, just that someone will need to invest more time than is presently available into figuring out the implications.

[debadutta98]

Hey, @wbt thank you for letting me know.

[debadutta98]

hi, @wbt @maverick1872 any update on this when this PR review is complete?

[maverick1872]

Additionally before I'd accept this I'd like to see test cases added to ensure that the guardrail functions as expected as well.

I had put this in a comment on our discussion on the PR itself but hadn't left a review. Which I'll do now.

[maverick1872]

Needs additional test cases to validate guardrail in my opinion.

[debadutta98]

Hi, @maverick1872 @wbt add some additional test cases.

[debadutta98]

Hi @maverick1872, I have added some test cases which need your review.

[DABH]

@debadutta98 Can you merge master into this branch? Might help with CI working. If it still fails, would need your help to fix failing tests :)

[debadutta98]

Hi, @DABH and @maverick1872 just do the clean up of this PR.

[debadutta98]

@DABH could you please run these test cases once again?

[debadutta98]

Hi @DABH, I haven't changed anything regarding this. I don't know why this is happening. can you please tell me the reason behind it?

[debadutta98]

Hi @DABH, I haven't changed anything regarding this. I don't know why this is happening. can you please tell me the reason behind it?

Hi, @DABH @maverick1872 @wbt can someone please tell me the issue here for which these test cases are failing?

[DABH]

I am not sure, this may require you to do a bit of digging and debugging… your help and time is greatly valued and appreciated if there’s anything you can do 🙏

[debadutta98]

Ok @DABH, i think this commit will resolve this issue finally. can you please rerun these test cases?

[DABH]

Seems like progress perhaps but maybe some new errors…

[maverick1872]

LGTM! Thanks for dropping the lint changes.

[debadutta98]

hi @maverick1872, can you please merge this one?