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

Comment metadata command #12

Open
Tracked by #5
gentlementlegen opened this issue May 14, 2024 · 10 comments
Open
Tracked by #5

Comment metadata command #12

gentlementlegen opened this issue May 14, 2024 · 10 comments

Comments

@gentlementlegen
Copy link
Member

gentlementlegen commented May 14, 2024

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 gentlementlegen changed the title Lastly, 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. Comment metadata command May 14, 2024
@gentlementlegen
Copy link
Member Author

gentlementlegen commented May 22, 2024

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

0x4007 commented Jun 17, 2024

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

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

0x4007 commented Jun 17, 2024

add the url related to the Action Run

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

@gentlementlegen
Copy link
Member Author

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

Keyrxng commented Jul 25, 2024

This is the last item in #5 after ubiquity-os-marketplace/assistive-pricing#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
Copy link
Contributor

0x4007 commented Jul 25, 2024

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

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

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

0x4007 commented Sep 24, 2024

@whilefoo can you get on the SDK?

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

No branches or pull requests

3 participants