feat(observability): otel integration add tracing #48

cchenggit · 2024-10-14T17:39:31Z

No description provided.

codecov · 2024-10-14T17:41:56Z

Codecov Report

Attention: Patch coverage is 74.70449% with 107 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
test/helper/helper.go	59.09%	12 Missing and 6 partials ⚠️
pkg/tracing/opentelemetry.go	79.22%	11 Missing and 5 partials ⚠️
db/dao/dao.go	62.50%	12 Missing and 3 partials ⚠️
pkg/tracing/tracing.go	73.21%	11 Missing and 4 partials ⚠️
pkg/queue/redis/redis.go	33.33%	8 Missing and 2 partials ⚠️
test/tracing/types.go	72.22%	9 Missing and 1 partial ⚠️
proxy/middlewares/recovery.go	0.00%	7 Missing ⚠️
config/database.go	75.00%	4 Missing and 2 partials ⚠️
config/config.go	42.85%	3 Missing and 1 partial ⚠️
app/app.go	76.92%	2 Missing and 1 partial ⚠️
... and 1 more

Flag	Coverage Δ
integration	`68.70% <72.81%> (+0.67%)`	⬆️
unit	`13.17% <5.53%> (-0.68%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
admin/admin.go	`71.42% <ø> (-0.99%)`	⬇️
admin/api/api.go	`93.54% <100.00%> (+0.21%)`	⬆️
admin/api/middleware.go	`96.00% <ø> (ø)`
config/redis.go	`100.00% <ø> (ø)`
config/tracing.go	`100.00% <100.00%> (ø)`
db/dao/attempt_dao.go	`100.00% <100.00%> (ø)`
db/dao/attempt_detail_dao.go	`91.17% <100.00%> (+1.52%)`	⬆️
db/db.go	`67.24% <100.00%> (+3.90%)`	⬆️
dispatcher/dispatcher.go	`76.85% <100.00%> (+0.99%)`	⬆️
pkg/taskqueue/redis.go	`84.40% <100.00%> (+2.48%)`	⬆️
... and 14 more

... and 1 file with indirect coverage changes

config.yml

proxy/router/router.go

pkg/tracing/opentelemetry/opentelemetry.go

vm-001 · 2024-10-23T03:01:30Z

Please resolve the conflict.

vm-001 · 2024-10-23T03:15:58Z

pkg/tracing/opentelemetry/opentelemetry.go

+
+type OpentelemetryConfig config.OpenTelemetryConfig
+
+func (o *OpentelemetryConfig) Setup(serviceName string, samplingRate float64, globalAttributes map[string]string) (trace.Tracer, io.Closer, error) {


I prefer to make OpentelemetryConfig as a function arg instead of a receiver.

interface should be better to implement different tracing setup.

pkg/tracing/opentelemetry/opentelemetry.go

config/tracing.go

vm-001 · 2024-10-23T03:21:35Z

config/tracing.go

+	CapturedResponseHeaders []string             `yaml:"captured_response_headers"`
+	SafeQueryParams         []string             `yaml:"safe_query_params"`
+	SamplingRate            float64              `yaml:"sampling_rate" default:"1"`
+}


How to specific the propagation type?

otel.SetTextMapPropagator(autoprop.NewTextMapPropagator()) will config propagation type from env OTEL_PROPAGATORS, default is tracecontext,baggage

config/tracing.go

vm-001 · 2024-10-25T16:13:38Z

config/tracing.go

+	ServiceName             string               `yaml:"service_name" default:"WebhookX"`
+	CapturedRequestHeaders  []string             `yaml:"captured_request_headers"`
+	CapturedResponseHeaders []string             `yaml:"captured_response_headers"`
+	SafeQueryParams         []string             `yaml:"safe_query_params"`


I don't see the safe_query_params is defined in the config.yml? do we really need it?

safe_query_params defined in config.yml, should check again.

vm-001 · 2024-10-25T16:15:17Z

config/tracing.go

+type OpenTelemetryConfig struct {
+	Protocol OtlpProtocol      `yaml:"protocol" envconfig:"PROTOCOL" default:"http/protobuf"`
+	Endpoint string            `yaml:"endpoint" envconfig:"ENDPOINT" default:"http://localhost:4318/v1/metrics"`
+	Headers  map[string]string `yaml:"headers,omitempty"`


Can we remove it as it can be configured via Otel Envs? Or do you think this is required?

it's necessary, webhookx should start up with a simple default config rather than letting user gain a deeper understanding about OTEL Envs.
It can be override by OTEL Envs if users have more requirements of tracing

go.mod

x

config.yml

vm-001 · 2024-10-26T09:02:27Z

Tracing instruments should be NOOP(as much as possible) when tracing.enabled is false to avoid performance impact.
DAO span name should be more detailed. e.g. dao.endpoints.list("dao.{opts.Table}.{operation}") instead of dao.list
Use otelsql.WithSpanOptions() to filter out unnecessary span such as sql.conn.reset_session.

cchenggit · 2024-10-27T06:58:55Z

Tracing instruments should be NOOP(as much as possible) when tracing.enabled is false to avoid performance impact.

tracing.enabled added, and it has been inject to database and redis config, so that they can control whether initialize tracing in the component.

DAO span name should be more detailed. e.g. dao.endpoints.list("dao.{opts.Table}.{operation}") instead of dao.list

good point, updated.

Use otelsql.WithSpanOptions() to filter out unnecessary span such as sql.conn.reset_session.

omited spans:

sql.connector.connect
sql.conn.ping
sql.conn.prepare
sql.conn.reset_session
sql.rows

all available spans list below:

MethodConnectorConnect Method = "sql.connector.connect"
MethodConnPing         Method = "sql.conn.ping"
MethodConnExec         Method = "sql.conn.exec"
MethodConnQuery        Method = "sql.conn.query"
MethodConnPrepare      Method = "sql.conn.prepare"
MethodConnBeginTx      Method = "sql.conn.begin_tx"
MethodConnResetSession Method = "sql.conn.reset_session"
MethodTxCommit         Method = "sql.tx.commit"
MethodTxRollback       Method = "sql.tx.rollback"
MethodStmtExec         Method = "sql.stmt.exec"
MethodStmtQuery        Method = "sql.stmt.query"
MethodRows             Method = "sql.rows"

vm-001 · 2024-10-27T06:54:16Z

admin/api/api.go

@@ -95,7 +97,7 @@ func (api *API) assert(err error) {
 func (api *API) Handler() http.Handler {
 	r := mux.NewRouter()

-	r.Use(panicRecovery)
+	r.Use(middlewares.PanicRecovery)


Despite that both panicRecovery in proxy and admin are shares most of logic, but I don't it's the right time to use the same middleware.

vm-001 · 2024-10-27T06:56:11Z

config/config.go

@@ -58,6 +59,10 @@ func (cfg Config) Validate() error {
 		return err
 	}



Suggested change

vm-001 · 2024-10-27T06:59:10Z

config/database.go

+	Username       string `yaml:"username" default:"webhookx"`
+	Password       string `yaml:"password" default:""`
+	Database       string `yaml:"database" default:"webhookx"`
+	tracingEnabled bool


I personally prefer replacing db driver at app initialize rather than having tracingEnabled everywhere.

vm-001 · 2024-10-27T07:00:10Z

config/redis.go

+	Port           uint32 `yaml:"port" default:"6379"`
+	Password       string `yaml:"password" default:""`
+	Database       uint32 `yaml:"database" default:"0"`
+	tracingEnabled bool


dispatcher/dispatcher.go

vm-001 · 2024-10-27T07:10:19Z

test/helper/helper.go

 func init() {
 	var err error
 	cfg, err = config.Init()
 	if err != nil {
 		panic(err)
 	}
-


Why does it need to be deleted?

IDE format issue

vm-001 · 2024-10-27T07:14:53Z

config/tracing.go

+type TracingConfig struct {
+	Enabled                 bool                 `yaml:"enabled" default:"false"`
+	Attributes              Map                  `yaml:"attributes"`
+	Opentelemetry           *OpenTelemetryConfig `yaml:"opentelemetry"`


I don't see any reason to use pointer here

vm-001 · 2024-10-27T07:24:05Z

config/tracing.go

+		return nil
+	}
+	if cfg.SamplingRate > 1 || cfg.SamplingRate < 0 {
+		return errors.New("invalid sampling rate, must be [0,1]")


Suggested change

return errors.New("invalid sampling rate, must be [0,1]")

return errors.New("sampling_rate must be in the range [0, 1]")

vm-001 · 2024-10-27T07:55:08Z

config/tracing.go

+type OpenTelemetryConfig struct {
+	Protocol OtlpProtocol `yaml:"protocol" envconfig:"PROTOCOL" default:"http/protobuf"`
+	Endpoint string       `yaml:"endpoint" envconfig:"ENDPOINT" default:"http://localhost:4318/v1/metrics"`
+	Headers  Map          `yaml:"headers" envconfig:"HEADERS"`


Let's remove Headers as it can be configured via
OTEL_EXPORTER_OTLP_TRACES_HEADERS

vm-001 · 2024-10-27T08:44:10Z

pkg/taskqueue/redis.go

@@ -91,6 +93,12 @@ func NewRedisQueue(opts RedisTaskQueueOptions, logger *zap.SugaredLogger, metric
 }

 func (q *RedisTaskQueue) Add(ctx context.Context, tasks []*TaskMessage) error {
+	if tracer := tracing.TracerFromContext(ctx); tracer != nil {
+		tracingCtx, span := tracer.Start(context.Background(), "queue.add", trace.WithSpanKind(trace.SpanKindClient))


Suggested change

tracingCtx, span := tracer.Start(context.Background(), "queue.add", trace.WithSpanKind(trace.SpanKindClient))

tracingCtx, span := tracer.Start(ctx, "queue.add", trace.WithSpanKind(trace.SpanKindClient))

Can we change queue.* to taskqueue.* as it may get confused with the proxy queue.

vm-001 · 2024-10-27T09:20:42Z

manually start tracing span in queue/redis/redis
The redisotel is basically covered by the manual span, so I think redisotel is not mandatory at this point, please remove it.

feat(observability): correct config feat(observability): fix otelsql tracing missing

cchenggit marked this pull request as draft October 14, 2024 17:39

vm-001 self-requested a review October 15, 2024 00:51

cchenggit force-pushed the feat/opentelemetry branch from be31dda to d9ab161 Compare October 22, 2024 14:12

cchenggit marked this pull request as ready for review October 22, 2024 14:28

vm-001 requested changes Oct 23, 2024

View reviewed changes

vm-001 reviewed Oct 23, 2024

View reviewed changes

pkg/tracing/opentelemetry/opentelemetry.go Outdated Show resolved Hide resolved

vm-001 reviewed Oct 23, 2024

View reviewed changes

config/tracing.go Show resolved Hide resolved

vm-001 reviewed Oct 23, 2024

View reviewed changes

config/tracing.go Show resolved Hide resolved

vm-001 reviewed Oct 23, 2024

View reviewed changes

feat(observability): support tracing otel

164487f

cchenggit force-pushed the feat/opentelemetry branch from 0d34f5f to 164487f Compare October 25, 2024 15:25

vm-001 requested changes Oct 25, 2024

View reviewed changes

vm-001 previously approved these changes Oct 25, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

vm-001 requested changes Oct 26, 2024

View reviewed changes

config.yml Outdated Show resolved Hide resolved

vm-001 requested changes Oct 27, 2024

View reviewed changes

vm-001 reviewed Oct 27, 2024

View reviewed changes

vm-001 added this to the 0.4.0 milestone Oct 27, 2024

cchenggit added 3 commits November 28, 2024 23:25

feat(observability): optimize review request changes

79ef232

feat(observability): correct config feat(observability): fix otelsql tracing missing

feat(observability): support otel tracing

c45a38e

feat:(observability): fix tracing order after otelsql

55b0ef5

cchenggit force-pushed the feat/opentelemetry branch from 9e1a1c5 to 55b0ef5 Compare November 28, 2024 17:35

fix: unit test with atomic.AddInt64 bug

3e9f617

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(observability): otel integration add tracing #48

feat(observability): otel integration add tracing #48

cchenggit commented Oct 14, 2024

codecov bot commented Oct 14, 2024 •

edited

Loading

vm-001 commented Oct 23, 2024

vm-001 Oct 23, 2024

cchenggit Oct 27, 2024

vm-001 Oct 23, 2024

cchenggit Oct 23, 2024

vm-001 Oct 25, 2024

cchenggit Oct 27, 2024

vm-001 Oct 25, 2024

cchenggit Oct 27, 2024

vm-001 commented Oct 26, 2024 •

edited

Loading

cchenggit commented Oct 27, 2024 •

edited

Loading

vm-001 Oct 27, 2024

vm-001 Oct 27, 2024

vm-001 Oct 27, 2024

vm-001 Oct 27, 2024 •

edited

Loading

vm-001 Oct 27, 2024

cchenggit Oct 27, 2024

vm-001 Oct 27, 2024

vm-001 Oct 27, 2024

vm-001 Oct 27, 2024

vm-001 Oct 27, 2024

vm-001 Oct 27, 2024 •

edited

Loading

vm-001 commented Oct 27, 2024 •

edited

Loading


		type OpentelemetryConfig config.OpenTelemetryConfig

		func (o *OpentelemetryConfig) Setup(serviceName string, samplingRate float64, globalAttributes map[string]string) (trace.Tracer, io.Closer, error) {

		@@ -58,6 +59,10 @@ func (cfg Config) Validate() error {
		return err
		}

	return errors.New("invalid sampling rate, must be [0,1]")
	return errors.New("sampling_rate must be in the range [0, 1]")

	tracingCtx, span := tracer.Start(context.Background(), "queue.add", trace.WithSpanKind(trace.SpanKindClient))
	tracingCtx, span := tracer.Start(ctx, "queue.add", trace.WithSpanKind(trace.SpanKindClient))

feat(observability): otel integration add tracing #48

Are you sure you want to change the base?

feat(observability): otel integration add tracing #48

Conversation

cchenggit commented Oct 14, 2024

codecov bot commented Oct 14, 2024 • edited Loading

Codecov Report

vm-001 commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vm-001 commented Oct 26, 2024 • edited Loading

cchenggit commented Oct 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vm-001 Oct 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vm-001 Oct 27, 2024 • edited Loading

Choose a reason for hiding this comment

vm-001 commented Oct 27, 2024 • edited Loading

codecov bot commented Oct 14, 2024 •

edited

Loading

vm-001 commented Oct 26, 2024 •

edited

Loading

cchenggit commented Oct 27, 2024 •

edited

Loading

vm-001 Oct 27, 2024 •

edited

Loading

vm-001 Oct 27, 2024 •

edited

Loading

vm-001 commented Oct 27, 2024 •

edited

Loading