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

proposal: add context.Context support #602

Open
AH-dark opened this issue May 14, 2023 · 11 comments
Open

proposal: add context.Context support #602

AH-dark opened this issue May 14, 2023 · 11 comments
Labels

Comments

@AH-dark
Copy link

AH-dark commented May 14, 2023

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

MikhailChe commented Sep 18, 2023

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

AH-dark commented Nov 17, 2023

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

demget commented Nov 19, 2023

Related to #222

@demget demget changed the title Pass additional data through context.Context in the process of message transmission proposal: add context.Context support Nov 19, 2023
@demget demget added the feature label Nov 19, 2023
@AH-dark
Copy link
Author

AH-dark commented Nov 20, 2023

@demget Do I need to provide an implementation?

@demget
Copy link
Collaborator

demget commented Nov 20, 2023

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

demget commented Nov 20, 2023

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

@demget demget added this to the v3.X milestone Nov 20, 2023
@demget demget pinned this issue Nov 20, 2023
@AH-dark
Copy link
Author

AH-dark commented Nov 20, 2023

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

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

ysomad commented May 10, 2024

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

ryadom commented Jun 15, 2024

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.

@anhnmt
Copy link

anhnmt commented Aug 10, 2024

+1

@demget demget removed this from the v3.X milestone Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants