chengxilo commented on PR #3464:
URL: https://github.com/apache/iggy/pull/3464#issuecomment-4722072237
> > nit: Just a minor stype recommendation For example:
> > ```go
> > c.logger.Info("Retrying to connect to server...", "retry_count",
n+1, "max_retries", attempts, "error", err)
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > can be changed to
> > ```go
> > c.logger.Info("Retrying to connect to server...",
> > slog.Uint64("retry_count", uint64(n+1)),
> > slog.Uint64("max_retries", uint64(attempts)),
> > slog.Any("error", err))
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > For detailed reason, check this reference:
https://www.dash0.com/guides/logging-in-go-with-slog#adding-contextual-attributes-to-your-logs
> > To be honest I think the previous approach may be more clear. But if we
have a lot of KVs, we probably need this strong but ugly approach.
> > I don't know why Go doesn't provide function like slog.Uint() for
convenience. 🤦♂️
>
> Thanks. I went with slog attributes.
>
> Also, one thing I noticed is that Rust's SDK has a connection state guard
in the `send_raw()` function before attempting any network operations. The Go
SDK doesn't have one in `sendWireAndFetchResponse()`. Unless I'm missing
something, it looks like calling an operation before `Connect()` succeeds could
eventually reach `c.conn.Write(...)` while `c.conn` is still nil and result in
a panic rather than `ErrNotConnected`. Do we need one here too?
Makes sense to me, just go ahead and add it : )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
