Chore/replace logs with logger

[wolfmic]

Resolves #4

As intended I replaced Warn with Error for now.
I can replace it with anything else later if required.

[Keyrxng]

The logger takes a string and then an object which is preferred over trying to include it in the string itself. This way we can pass any sort of metadata and we never see [Object object]

I would refactor back to the original way of passing objects as a second param. If it makes sense to include a single variable in the string do that but it's also good to just wrap that var in an object too

[gentlementlegen]

The logger takes a string and then an object which is preferred over trying to include it in the string itself. This way we can pass any sort of metadata and we never see [Object object]

I would refactor back to the original way of passing objects as a second param. If it makes sense to include a single variable in the string do that but it's also good to just wrap that var in an object too

Indeed the problem you mention can be seen here. However does the second argument of the logger actually get logged somewhere? I don't remember if that is the current behavior.

[Keyrxng]

The logger takes a string and then an object which is preferred over trying to include it in the string itself. This way we can pass any sort of metadata and we never see [Object object]
I would refactor back to the original way of passing objects as a second param. If it makes sense to include a single variable in the string do that but it's also good to just wrap that var in an object too

Indeed the problem you mention can be seen here. However does the second argument of the logger actually get logged somewhere? I don't remember if that is the current behavior.

Yeah it does see this run. In my opinion it looks like it's treated as a second call to actually log it (that may be wrong and it's just how the formatting works) but it's effective and easily read. code reference

[wolfmic]

I applied some changes and I will check later the arguments to add later.

I might need to use the function from start-stop project: createStructuredMetadata, it might be a good step to achieve it.

On another hand, logger.ok returns LogReturn, the previous logger returns Promise.
I kinda wonder how to handle that part.

I will recode the comment creation later.

[Keyrxng]

I applied some changes and I will check later the arguments to add later.

I might need to use the function from start-stop project: createStructuredMetadata, it might be a good step to achieve it.

On another hand, logger.ok returns LogReturn, the previous logger returns Promise. I kinda wonder how to handle that part.

I will recode the comment creation later.

No no you shouldn't need that function. That's to create a HTML comment which is posted onto the issue. We just want to log any additional info that we can by passing an object to any Logs function after the initial string.

LogReturn contains two versions of the message being passed in which is used for posting the comments to GitHub amongst other things and each Logs function returns this.

The previous logger was just a placeholder. It's logic or whatever it returned should be replaced with what the real Logs returns.

Look here for one way that we handle using the LogReturn object for reference. We are adopting a 'bubble all errors up to the entry' approach slowly but surely

[0x4007]

Doesn't look right. The latter parameter is for JSON metadata. This is important for filtering/sorting the error class (the string) and then investigating the details (the metadata) in the SQL database

[wolfmic]

Oh I see. Then, if I keep locationMetaData as a parameter, I need it to be a Partial, it does not match the locationMetaData type.
If I understand it well, I would need the logger method "debug" to handle locationMetaData ?

Sorry in advance for my questions, I am not well away of all the types used in the project:
The two types don't really match in this current state.

[Keyrxng]

wrap anything you pass as a second param in an object so that locationMetaData is a key of {}

[0x4007]

Perhaps I'm missing some large detail here but this used to write to our Supabase. I would expect this to be async.

[Keyrxng]

Yeah we removed all GitHub and Supabase integrations from the logger because it was dangerous to standardize it into plugins and have it be able to post directly to our db.

Also isn't it the case that we should only store to Supabase on .fatal() and only from inside either the kernel or core plugins? This is a core plugin but in general I mean to confirm

I have suggested that the current logger implementation could be for non-core plugins and we have a core-logger which has the ability to post to our Supabase? Alternatively refactor the current logger but any way we refactor it opens us up to the issues we sought to resolve

[0x4007]

We can also post fatal to a dedicated telegram chat for us to triage. It might be simpler and more effective than a database specifically to triage errors.

Definitely for kernel. Not sure for plugins.

[0x4007]

Seems unusual to coerce to a string with this syntax.

[wolfmic]

I see, would it be better in this form ?
context.logger.error("", { error: e as Error });
Would it be necessary in that case to add a string as well?

[Keyrxng]

Typically we add a string yeah, avoid empty strings as much as possible.

context.logger.error("Error string", { e });

no need to cast as Error if you just pass the e object

[0x4007]

Probably can just pass e object straight in instead of nesting inside another object.

[gentlementlegen]

I think the type expects to get the error within an object and not the error straight in, at least with our current version of the logger. Could be changed later.

[wolfmic]

If I must summarise what I must implement to get this PR done:

• creating comment through octokit
  • Should I write a file for that the same way start-stop is handling it with addCommentToIssue ? as @Keyrxng suggested here ? Chore/replace logs with logger #15 (comment)

Do I miss something in the whole logic?