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

Add guardrails to Profiler class #2226

Merged
merged 10 commits into from
Aug 9, 2023
Merged

Conversation

debadutta98
Copy link
Contributor

@debadutta98 debadutta98 commented Oct 29, 2022

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
Copy link
Contributor

wbt commented Oct 31, 2022

Can you say a few words about backward compatibility?

@debadutta98
Copy link
Contributor Author

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
Copy link
Contributor

wbt commented Nov 4, 2022

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
Copy link
Member

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.

@debadutta98 debadutta98 closed this Apr 1, 2023
@wbt
Copy link
Contributor

wbt commented Apr 3, 2023

@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.

@wbt wbt reopened this Apr 3, 2023
@debadutta98
Copy link
Contributor Author

Hey, @wbt thank you for letting me know.

@debadutta98
Copy link
Contributor Author

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

@maverick1872
Copy link
Member

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.

Copy link
Member

@maverick1872 maverick1872 left a comment

Choose a reason for hiding this comment

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

Needs additional test cases to validate guardrail in my opinion.

test/unit/winston/profiler.test.js Outdated Show resolved Hide resolved
@debadutta98
Copy link
Contributor Author

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

@debadutta98
Copy link
Contributor Author

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

@DABH
Copy link
Contributor

DABH commented Jun 29, 2023

@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
Copy link
Contributor Author

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

@debadutta98
Copy link
Contributor Author

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

@debadutta98
Copy link
Contributor Author

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?

image

@debadutta98
Copy link
Contributor Author

debadutta98 commented Jun 30, 2023

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?

image

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

@DABH
Copy link
Contributor

DABH commented Jun 30, 2023

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
Copy link
Contributor Author

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

@DABH
Copy link
Contributor

DABH commented Jun 30, 2023

Seems like progress perhaps but maybe some new errors…

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.

[Bug]: Profiler constructor has no guardrails for what it's constructed from
5 participants