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

feat: worker log and revision #1

Conversation

Keyrxng
Copy link

@Keyrxng Keyrxng commented Nov 5, 2024

@Keyrxng
Copy link
Author

Keyrxng commented Nov 5, 2024

obviously feel free to adjust whatever you think is best in terms of what we put in the header but we have all the components we need to fill it

So after this is merged into the SDK and I adjust worker.deploy to push the secret for each plugin then it'll work in all plugins

Defined at the SDK level so plugins don't need to define it.

if (pluginOptions.postCommentOnError && loggerError) {
await postComment(context, loggerError);
await postComment(context, loggerError, honoEnvironment);
Copy link
Author

@Keyrxng Keyrxng Nov 5, 2024

Choose a reason for hiding this comment

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

I've passed it as a param here instead of building it into context because the SDK consumer can access anything in context and if they installed their worker into any config other than their own they'd be able to exfiltrate the api token of the partner or us, I think, maybe not.

At least that was my intention, I hope that it holds true.

@@ -80,13 +80,16 @@ export async function createActionsPlugin<TConfig = unknown, TEnv = unknown, TSu
env = process.env as TEnv;
}


Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change

@@ -80,13 +80,16 @@ export async function createActionsPlugin<TConfig = unknown, TEnv = unknown, TSu
env = process.env as TEnv;
}



Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change

@gentlementlegen
Copy link
Owner

@Keyrxng thank you for that. Does it work properly within Actions as well?

@Keyrxng
Copy link
Author

Keyrxng commented Nov 6, 2024

@Keyrxng thank you for that. Does it work properly within Actions as well?

I didn't QA it for actions, I thought you would of QA'd and merged this in and adjusted things

Probs just copy paste the logic into the workflow postComment I guess?

If there is any post merge comment I'll take care of it as well.

I'm not sure if you mean you'll catch anything from this PR to adjust and merge it in or something else, I'm pretty tired so if it's still to be done later I'll catch it np

@gentlementlegen
Copy link
Owner

@Keyrxng the postComment should only have one function, that adapts given the context where it runs, so if I call postComment from an action or a worker it should be the same import / same function call, this way it makes dev experience much simpler.

@Keyrxng
Copy link
Author

Keyrxng commented Nov 6, 2024

Yeah I saw that they were duplicated for worker/action.

I assumed the reason was octokit.core and keeping those imports seperate as it usually causes issues for workers.

Why was the postComment and the rest of the createPlugin/createActionPlugin logic separated in the first place, was it something to do with imports or just for "modularity" or something?

I even added a similar comment in ubiquity-os#191 that would allow the proxy callbacks to detect and handle worker/action specifics, but the SDK has been built to keep their intended logic separate...

@Keyrxng
Copy link
Author

Keyrxng commented Nov 6, 2024

I'll just PR the kernel

@Keyrxng Keyrxng closed this Nov 6, 2024
@Keyrxng
Copy link
Author

Keyrxng commented Nov 6, 2024

Was gonna QA actions and submit a PR but fell into a rabbit hole exploring what's possible with the Error util fns and class options lmao. Will finish it up tomorrow after my uncertainty on where to actually PR is cleared up

I have a clear understanding of it and solutions to hand but would like your opinion

It more or less boils down to this:

class CustomError extends Error implements LogReturn {
    logMessage: { raw: string; diff: string; level: LogLevel; type: LogLevelWithOk; };
    metadata?: Metadata | undefined;

    constructor(log: LogReturn) {
        super(log.logMessage.raw);
        Error.captureStackTrace(this, this.constructor);
        this.logMessage = log.logMessage;
        this.metadata = log.metadata;
    }
}

export class Logger extends Logs {
    constructor(logLevel: LogLevel) {
        super(logLevel);
    }

    debug(log: string, metadata?: Metadata): LogReturn {
        return new CustomError(super.debug(log, metadata));
    }
    ...


const context: Context<TConfig, TEnv, TSupportedEvents> = {
 logger: new Logger(pluginOptions.logLevel),
}

Does it make sense for me to PR against the logger or against the SDK? I'm unsure because of:

  1. SDK contains everything; a consumer does not init a logger anymore.
  2. Having a separate internal logger with adjustments just for the kernel might make sense, like it having posting features the pkg logger shouldn't have etc?
  3. The logger pkg could be deprecated and we just use the SDK which exports the logger, centralizing everything into one package sounds nice imo.
  4. For posting the stack to github should the content be optimized e.g remove everything but the last three dirs in the file path instead of seeing drives and user profiles etc?
  5. Should we remove the stack from metadata and format it with MD instead of JSON.Stringify when posting to GH?

@gentlementlegen
Copy link
Owner

I think we can keep is simple, the kernel has a logger and so does the SDK (which is provided to the plugin instance). What is important I think is cross operability of the SDK no matter what its context is (worker, action etc) so no code changes are needed. My aim here is to improve dev experience.

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.

Improve Cloudflare Worker postComment metadata
2 participants