ubiquity-os / plugins-wishlist

0 stars 2 forks source link

Comment metadata command #12

Open gentlementlegen opened 4 months ago

gentlementlegen commented 4 months ago

The bot comment metadata system is very useful for error logs, but sometimes the error logs dont show enough information and this should be fixed.

See source code

gentlementlegen commented 4 months ago

@0x4007 Was thinking maybe there might be a little more user friendly approach to this.

What if instead of pushing this in the metadata, we just output the error message with a collapsible stack trace / metadata beneath it? Because either way the metadata is readable by everyone afaik, but is just a plain string within the HTML. If we instead do it inside the Issue comment, we could benefit from formatting, copy paste etc.

Second concern was security, What if by accident we output sensitive data? So I thought we could instead just put a link to the record in the database since we push logs there. Problem is not everyone has access to it so not everyone could see the output, which is not ideal.

Third I think this might be better located in the Kernel since it knows which plugin invocation has succeeded or fail and could even aggregate them.

0x4007 commented 3 months ago

Because either way the metadata is readable by everyone afaik

No only collaborators. Metadata can have sensitive information.

we could benefit from formatting, copy paste etc

The idea for metadata was that it is intended to be easily parsable by diagnostics tools.

To be honest, the original inspiration was due to us not wanting to rely on a database. As our system needs became more complex it seemed to be unavoidable. However storing all the data we need on GitHub and its comments makes our infrastructure significantly leaner and easier to maintain.

gentlementlegen commented 3 months ago

@0x4007 All right makes sense. I think a nice to have would be to add the url related to the Action Run or Supabase logs if any, to avoid looking for the related run. Will look into it.

0x4007 commented 3 months ago

add the url related to the Action Run

Agreed but it wasn't super easy to implement as far as I remember.

gentlementlegen commented 2 months ago

Now that the package https://github.com/ubiquity/ubiquibot-logger has been updated to be decoupled from the Probot used in v1, I think this task because easier. However, this package has to be included in every plugin created until now, so quite tedious.

The new logger is able to produce pretty logs and the metadata, that would need to be posted with the comments.

Keyrxng commented 2 months ago

This is the last item in https://github.com/ubiquibot/plugins-wishlist/issues/5 after https://github.com/ubiquibot/assistive-pricing/pull/18

Is it possible to refine this spec a bit further I think it seems outdated/not super clear

Essentially this is going to work in unison with ubiquibot-logger and is responsible for posting hidden HTML comments for typically errors but I guess any type of bot comment?

add the url related to the Action Run

What's the case for if it's a worker?

No only collaborators. Metadata can have sensitive information.

I remember mentioning that an org could have a private storage repo that plugins could use as their storage solution, it could also include error logs which seems like a good solution here but I know originally the idea wasn't so attractive.

0x4007 commented 2 months ago

I am open to enforcing "as a best practice" that we don't post sensitive information with the logger but seems a bit risky.

gentlementlegen commented 2 months ago

Metadata should be handled on a plugin basis because not every plugin posts comments to GitHub. And eventually posting itself can fail, which in such case should be handled by the kernel that should register the error within out Supabase instance (these is a task for that in the kernel). I added metadata to conversation-rewards comments but I guess this ticket is applicable to any plugin / worker posting comments.

gentlementlegen commented 1 day ago

I think now that we started developing a SDK, it would be a good fit there. The problem with v1 is that it used an exec to get the HEAD sha, but exec is forbidden in workers for security reasons.

0x4007 commented 19 hours ago

@whilefoo can you get on the SDK?