tucnak / telebot

Telebot is a Telegram bot framework in Go.
MIT License
3.8k stars 446 forks source link

proposal: add context.Context support #602

Open AH-dark opened 1 year ago

AH-dark commented 1 year ago

In the process of using in a production environment, we often need to trace the source and operation of each request, that is, link tracing.

In the current scenario, although telebot has implemented a handler mode similar to http, it is completely unadaptable to link tracing. It would be much better if we just add context.Context to the Update object or try to pass messages not through Update but directly through *Context.

I think this is a problem worth considering. When I tried to combine this package with microservice systems and message queues to improve message processing efficiency and concurrency capabilities, I found that it could not associate the message context, which is fatal.

MikhailChe commented 10 months ago

Inspired by go-chi/chi, I attempted to redesign handlers and middlewares to accept the stdlib context.Context. You can find the changes in the following GitHub repository: https://github.com/MikhailChe/telebot/tree/mikhailche-context-first-effort

I am currently testing it, and at the moment, it feels a bit strange. I am considering moving telebot.Context as a value inside the context.Context with some helper functions to retrieve it from there.

Unfortunately, this redesign requires rewriting all currently implemented handlers and middlewares and might remain as a separate project indefinitely (or perhaps become part of telebot v4? :))

AH-dark commented 8 months ago

I suggest you follow the example of github.com/cloudwego/hertz. Pass context.Context as the first parameter and telebot.Context as the second parameter.

demget commented 7 months ago

Related to #222

AH-dark commented 7 months ago

@demget Do I need to provide an implementation?

demget commented 7 months ago

I don't really like the idea of pasting the context everywhere by default, it's not that useful for small single bots, and we must keep the backward compatibility.

It would be great to define the main points on where and why the context should be included, and think of how can we provide a clean non-breaking implementation.

Your help is greatly appreciated. I'm adding this issue to the future releases milestone.

demget commented 7 months ago

@MikhailChe @AH-dark do you have any solid examples of using the forked telebot versions with context.Context included?

AH-dark commented 7 months ago

@MikhailChe @AH-dark do you have any solid examples of using the forked telebot versions with context.Context included?

I agree with your view, but it needs to be pointed out that adding a context would not be a burden for the small bot, whereas not adding a context would prevent us from using telebot to build large-scale robots.

Additionally, as far as I know, people usually use Python instead of Go to build small bots. 🤔

MikhailChe commented 7 months ago

@demget, I have a small pet project that uses a forked version for tracing. This allows me to pass "spans" via context to the http client. It's a bit sloppy, but it is a real working example.

ysomad commented 2 months ago

Will be very useful, in my project im using slog and custom slog.Handler implementation where in Handle method passing value to slog.Record from context such as trace-id, and other user or request specific fields. It allows me not to lose potential log details further I go into code abstractions and also allows code to be more "clean".

For now I'm just creating context.Context in middleware on every request and passing it to tele.Context and then passing around into logs, context in context ikr, but it works for lack of an alternative

func (h *handlerMiddleware) Handle(ctx context.Context, req slog.Record) error {
    if c, ok := ctx.Value(ctxKey{}).(logCtx); ok {
        if c.Recipient != "" {
            req.Add("recipient", c.Recipient)
        }
        if c.Version != "" {
            req.Add("version", c.Version)
        }
    }

    return h.next.Handle(ctx, req)
}
ryadom commented 1 month ago

Thumbs up. I would like to integrate open telemetry to the bot for observability. I would like to see context.Context there for implementing it.