feedback from users

[aldas]

Do we need additional features?

---

From prometheus middleware experience I assume there will be some people that want custom metrics support or disable/replace built-in metrics.

Additional metrics can be incremented in echootel.Config.MetricAttributes callback and Metrics attributes can be modified/added in that same callback. This is not very elequant solution but maybe we do not anything more complex than this

	e.Use(echootel.NewMiddlewareWithConfig(echootel.Config{
		MetricAttributes: func(c *echo.Context, v *echootel.Values, attr []attribute.KeyValue) []attribute.KeyValue {
			// increment a metric here
			return append(attr, attribute.String("custom_attr", "value"))
		},
	}))

edit:

custom metrics can be provided by implementing this interface

echo-opentelemetry/otel.go

Lines 92 to 96 in a98d1db

	// MetricsRecorder is used to record metrics. This interface is used to allow custom metrics recording with access to
	// the Echo context, so additional attributes can be extracted from it.
	type MetricsRecorder interface {
	Record(c *echo.Context, v RecordValues)
	}

default implementation

[vadimi]

thanks @aldas, I've tried pulling the new package into one of my projects. A couple of things I've noticed immediately:

1. The import path github.com/labstack/echo-opentelemetry/echootel like in the example doesn't work, it needs to be echootel "github.com/labstack/echo-opentelemetry" since the root package name is different from the repo/module name. Is that intentional? Maybe having all code under echootel directory would look better?
2. Scope name in the otel.go still uses the other repo name: ScopeName = "github.com/labstack/echo-contrib/echootel". Should it reflect the new repo name?

[aldas]

1. echootel "github.com/labstack/echo-opentelemetry" is correct. This is similar to Echo JWT library usage 1. At least in IntelliJ IDEA if I do go get github.com/labstack/echo-opentelemetry I only need to type echootel. and it starts to offer things from that repo.

2. Yep, seems that I forgot that.

[vadimi]

tested it a bit more and noticed there is a default OnError handler, which logs otel middleware error. I'm already using HTTPErrorHandler from echo, so this one looks a bit redundant for me. While OnError hook is definitely useful in the middleware, having the default one that just logs an error seems not super important.

[aldas]

which logs otel middleware error

For some of the cases these are actually only warnings.

echo-opentelemetry/otel.go

Lines 177 to 179 in bdd9b1e

	if err := ev.ExtractRequest(request); err != nil {
	config.OnError(c, err)
	}

here it is actual error

echo-opentelemetry/otel.go

Lines 217 to 221 in bdd9b1e

	if err != nil {
	span.SetAttributes(semconv.ErrorType(err))
	span.SetStatus(codes.Error, err.Error())
	config.OnError(c, err)
	}

You made a valid point. I'll remove default callback.

[aldas]

I have separated these two places to different callback. I did not want to ignore extraction error although most of the time this would be ok.

@vadimi do current API, naming etc seems ok for you? If it is, will do first release and see how it all plays out.

[vadimi]

@aldas yes, everything seems to be fine for now.

[aldas]

v0.0.1 is released.

[aldas]

note to self: open-telemetry/opentelemetry-go-contrib#4580 (comment)