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 prefix string to error log #75

Merged
merged 2 commits into from
Aug 2, 2024
Merged

Add prefix string to error log #75

merged 2 commits into from
Aug 2, 2024

Conversation

tedyu
Copy link
Contributor

@tedyu tedyu commented Apr 5, 2024

This PR adds prefix string when error is encountered.

In elk, we sometimes saw the follwing:

 TypeError: removeMarkdown(...).substr is not a function

It might be possible that removeMarkdown wasn't able to parse the argument.

However, there is not much clue what error happened.

With the error prefix, it would be easier to find the cause.

@tedyu
Copy link
Contributor Author

tedyu commented Apr 5, 2024

Alternatively we can throw the error.
To keep backward compatibility, we can add option (default to false) which allows the error to be thrown out.

@hongaar
Copy link
Contributor

hongaar commented Apr 5, 2024

How would this change help you with triaging your issue?

Alternatively, could you add a breakpoint and step through the code (of remove-markdown) to pinpoint the line the error occurs in?

@tedyu
Copy link
Contributor Author

tedyu commented Apr 5, 2024

@hongaar
I added an option, throwError (default false), to keep backward compatibility.

add a breakpoint and step through the code
Since the TypeError happened in production and we don't have the input that triggered the situation, currently we cannot debug this way.
Once the PR is accepted, we can upgrade to the next release and obtain the input for further debugging.

cc @zuchka

Thanks

@tedyu
Copy link
Contributor Author

tedyu commented Apr 8, 2024

@zuchka
Can you take a look ?
Thanks

@tedyu
Copy link
Contributor Author

tedyu commented Apr 10, 2024

Gentle ping, @zuchka

@stiang
Copy link
Collaborator

stiang commented Apr 10, 2024

@tedyu I’m not really involved with this repo anymore, but if you are struggling to get your PR merged, I would suggest that you temporarily point your production setup to the fork with the PR in it (most dependency resolvers support pointing directly to a GitHub branch), figure out what caused the exception using your new option to throw errors, and then maybe switch back to the regular package. I see no reason to wait for this to get merged to figure out what is causing the bug, now that you already have a fork that can help.

@zuchka
Copy link
Owner

zuchka commented Aug 2, 2024

merged! so sorry for the delay

@zuchka zuchka merged commit b770b23 into zuchka:main Aug 2, 2024
@zuchka
Copy link
Owner

zuchka commented Aug 2, 2024

shall we do some more testing before we cut a new release, @tedyu?

@tedyu
Copy link
Contributor Author

tedyu commented Aug 2, 2024

This PR seems pretty safe.
Looking forward for the new release containing this PR.

Thanks

@zuchka
Copy link
Owner

zuchka commented Aug 3, 2024

I'll cut a new release in the next day or so 👍

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.

None yet

4 participants