Merge branch 'main' into nv/4074

pkg/modules/metricsexplorer/implmetricsexplorer/module.go

@@ -217,12 +217,6 @@ func (m *module) GetTreemap(ctx context.Context, orgID valuer.UUID, req *metrics
 }
 
 func (m *module) GetMetricMetadataMulti(ctx context.Context, orgID valuer.UUID, metricNames []string) (map[string]*metricsexplorertypes.MetricMetadata, error) {
-
-	comment := ctxtypes.CommentFromContext(ctx)
-	comment.Set("signal", telemetrytypes.SignalMetrics.StringValue())
-	comment.Set("is_metadata_query", "true")
-	ctx = ctxtypes.NewContextWithComment(ctx, comment)
-
 	if len(metricNames) == 0 {
 		return map[string]*metricsexplorertypes.MetricMetadata{}, nil
 	}

pkg/querier/builder_query.go

@@ -12,7 +12,6 @@ import (
 	"github.com/SigNoz/signoz/pkg/errors"
 	"github.com/SigNoz/signoz/pkg/telemetrylogs"
 	"github.com/SigNoz/signoz/pkg/telemetrystore"
-	"github.com/SigNoz/signoz/pkg/types/ctxtypes"
 	qbtypes "github.com/SigNoz/signoz/pkg/types/querybuildertypes/querybuildertypesv5"
 	"github.com/SigNoz/signoz/pkg/types/telemetrytypes"
 	"github.com/bytedance/sonic"
@@ -213,12 +212,6 @@ func (q *builderQuery[T]) Execute(ctx context.Context) (*qbtypes.Result, error)
 
 // executeWithContext executes the query with query window and step context for partial value detection
 func (q *builderQuery[T]) executeWithContext(ctx context.Context, query string, args []any) (*qbtypes.Result, error) {
-
-	comment := ctxtypes.CommentFromContext(ctx)
-	comment.Set("signal", q.spec.Signal.StringValue())
-	comment.Set("duration", qbtypes.DurationBucket(q.fromMS, q.toMS))
-	ctx = ctxtypes.NewContextWithComment(ctx, comment)
-
 	totalRows := uint64(0)
 	totalBytes := uint64(0)
 	elapsed := time.Duration(0)

pkg/querier/clickhouse_query.go

@@ -14,7 +14,6 @@ import (
 	"github.com/SigNoz/signoz/pkg/errors"
 	"github.com/SigNoz/signoz/pkg/querybuilder"
 	"github.com/SigNoz/signoz/pkg/telemetrystore"
-	"github.com/SigNoz/signoz/pkg/types/ctxtypes"
 	qbtypes "github.com/SigNoz/signoz/pkg/types/querybuildertypes/querybuildertypesv5"
 )
 
@@ -100,10 +99,6 @@ func (q *chSQLQuery) renderVars(query string, vars map[string]qbtypes.VariableIt
 
 func (q *chSQLQuery) Execute(ctx context.Context) (*qbtypes.Result, error) {
 
-	comment := ctxtypes.CommentFromContext(ctx)
-	comment.Set("duration", qbtypes.DurationBucket(q.fromMS, q.toMS))
-	ctx = ctxtypes.NewContextWithComment(ctx, comment)
-
 	totalRows := uint64(0)
 	totalBytes := uint64(0)
 	elapsed := time.Duration(0)

pkg/querier/promql_query.go

@@ -14,8 +14,6 @@ import (
 	"github.com/SigNoz/signoz/pkg/errors"
 	"github.com/SigNoz/signoz/pkg/prometheus"
 	"github.com/SigNoz/signoz/pkg/querybuilder"
-	"github.com/SigNoz/signoz/pkg/types/ctxtypes"
-	qbtypes "github.com/SigNoz/signoz/pkg/types/querybuildertypes/querybuildertypesv5"
 	qbv5 "github.com/SigNoz/signoz/pkg/types/querybuildertypes/querybuildertypesv5"
 	"github.com/SigNoz/signoz/pkg/types/telemetrytypes"
 	"github.com/prometheus/prometheus/promql"
@@ -189,11 +187,6 @@ func (q *promqlQuery) renderVars(query string, vars map[string]qbv5.VariableItem
 
 func (q *promqlQuery) Execute(ctx context.Context) (*qbv5.Result, error) {
 
-	comment := ctxtypes.CommentFromContext(ctx)
-	comment.Set("signal", telemetrytypes.SignalMetrics.StringValue())
-	comment.Set("duration", qbtypes.DurationBucket(q.tr.From, q.tr.To))
-	ctx = ctxtypes.NewContextWithComment(ctx, comment)
-
 	start := int64(querybuilder.ToNanoSecs(q.tr.From))
 	end := int64(querybuilder.ToNanoSecs(q.tr.To))
 

pkg/querier/querier.go

@@ -16,7 +16,6 @@ import (
 	"github.com/SigNoz/signoz/pkg/query-service/utils"
 	"github.com/SigNoz/signoz/pkg/querybuilder"
 	"github.com/SigNoz/signoz/pkg/telemetrystore"
-	"github.com/SigNoz/signoz/pkg/types/ctxtypes"
 	"github.com/SigNoz/signoz/pkg/types/metrictypes"
 	"github.com/SigNoz/signoz/pkg/types/telemetrytypes"
 	"golang.org/x/exp/maps"
@@ -527,12 +526,6 @@ func (q *querier) run(
 	steps map[string]qbtypes.Step,
 	qbEvent *qbtypes.QBEvent,
 ) (*qbtypes.QueryRangeResponse, error) {
-
-	comment := ctxtypes.CommentFromContext(ctx)
-	comment.Set("panel_type", qbEvent.PanelType)
-	comment.Set("query_type", qbEvent.QueryType)
-	ctx = ctxtypes.NewContextWithComment(ctx, comment)
-
 	results := make(map[string]any)
 	warnings := make([]string, 0)
 	warningsDocURL := ""

pkg/signoz/provider.go

@@ -176,10 +176,9 @@ func NewSQLMigrationProviderFactories(
 func NewTelemetryStoreProviderFactories() factory.NamedMap[factory.ProviderFactory[telemetrystore.TelemetryStore, telemetrystore.Config]] {
 	return factory.MustNewNamedMap(
 		clickhousetelemetrystore.NewFactory(
-			telemetrystorehook.NewLoggingFactory(),
-			// adding instrumentation factory before settings as we are starting the query span here
-			telemetrystorehook.NewInstrumentationFactory(),
 			telemetrystorehook.NewSettingsFactory(),
+			telemetrystorehook.NewLoggingFactory(),
+			telemetrystorehook.NewInstrumentationFactory(),
 		),
 	)
 }

pkg/telemetrymetadata/metadata.go

@@ -13,7 +13,6 @@ import (
 	"github.com/SigNoz/signoz/pkg/telemetrymetrics"
 	"github.com/SigNoz/signoz/pkg/telemetrystore"
 	"github.com/SigNoz/signoz/pkg/telemetrytraces"
-	"github.com/SigNoz/signoz/pkg/types/ctxtypes"
 	"github.com/SigNoz/signoz/pkg/types/metrictypes"
 	qbtypes "github.com/SigNoz/signoz/pkg/types/querybuildertypes/querybuildertypesv5"
 	"github.com/SigNoz/signoz/pkg/types/telemetrytypes"
@@ -140,12 +139,6 @@ func (t *telemetryMetaStore) tracesTblStatementToFieldKeys(ctx context.Context)
 
 // getTracesKeys returns the keys from the spans that match the field selection criteria
 func (t *telemetryMetaStore) getTracesKeys(ctx context.Context, fieldKeySelectors []*telemetrytypes.FieldKeySelector) ([]*telemetrytypes.TelemetryFieldKey, bool, error) {
-
-	comment := ctxtypes.CommentFromContext(ctx)
-	comment.Set("signal", telemetrytypes.SignalTraces.StringValue())
-	comment.Set("is_metadata_query", "true")
-	ctx = ctxtypes.NewContextWithComment(ctx, comment)
-
 	if len(fieldKeySelectors) == 0 {
 		return nil, true, nil
 	}
@@ -341,12 +334,6 @@ func (t *telemetryMetaStore) logsTblStatementToFieldKeys(ctx context.Context) ([
 
 // getLogsKeys returns the keys from the spans that match the field selection criteria
 func (t *telemetryMetaStore) getLogsKeys(ctx context.Context, fieldKeySelectors []*telemetrytypes.FieldKeySelector) ([]*telemetrytypes.TelemetryFieldKey, bool, error) {
-
-	comment := ctxtypes.CommentFromContext(ctx)
-	comment.Set("signal", telemetrytypes.SignalLogs.StringValue())
-	comment.Set("is_metadata_query", "true")
-	ctx = ctxtypes.NewContextWithComment(ctx, comment)
-
 	if len(fieldKeySelectors) == 0 {
 		return nil, true, nil
 	}
@@ -596,12 +583,6 @@ func getPriorityForContext(ctx telemetrytypes.FieldContext) int {
 
 // getMetricsKeys returns the keys from the metrics that match the field selection criteria
 func (t *telemetryMetaStore) getMetricsKeys(ctx context.Context, fieldKeySelectors []*telemetrytypes.FieldKeySelector) ([]*telemetrytypes.TelemetryFieldKey, bool, error) {
-
-	comment := ctxtypes.CommentFromContext(ctx)
-	comment.Set("signal", telemetrytypes.SignalMetrics.StringValue())
-	comment.Set("is_metadata_query", "true")
-	ctx = ctxtypes.NewContextWithComment(ctx, comment)
-
 	if len(fieldKeySelectors) == 0 {
 		return nil, true, nil
 	}
@@ -704,12 +685,6 @@ func (t *telemetryMetaStore) getMetricsKeys(ctx context.Context, fieldKeySelecto
 
 // getMeterKeys returns the keys from the meter metrics that match the field selection criteria
 func (t *telemetryMetaStore) getMeterSourceMetricKeys(ctx context.Context, fieldKeySelectors []*telemetrytypes.FieldKeySelector) ([]*telemetrytypes.TelemetryFieldKey, bool, error) {
-
-	comment := ctxtypes.CommentFromContext(ctx)
-	comment.Set("signal", telemetrytypes.SignalMetrics.StringValue())
-	comment.Set("is_metadata_query", "true")
-	ctx = ctxtypes.NewContextWithComment(ctx, comment)
-
 	if len(fieldKeySelectors) == 0 {
 		return nil, true, nil
 	}
@@ -1646,10 +1621,6 @@ func (t *telemetryMetaStore) FetchTemporalityMulti(ctx context.Context, queryTim
 }
 
 func (t *telemetryMetaStore) FetchTemporalityAndTypeMulti(ctx context.Context, queryTimeRangeStartTs, queryTimeRangeEndTs uint64, metricNames ...string) (map[string]metrictypes.Temporality, map[string]metrictypes.Type, error) {
-	comment := ctxtypes.CommentFromContext(ctx)
-	comment.Set("signal", telemetrytypes.SignalMetrics.StringValue())
-	comment.Set("is_metadata_query", "true")
-	ctx = ctxtypes.NewContextWithComment(ctx, comment)
 	if len(metricNames) == 0 {
 		return make(map[string]metrictypes.Temporality), make(map[string]metrictypes.Type), nil
 	}

pkg/telemetrymetrics/statement_builder.go

@@ -159,6 +159,23 @@ func (b *MetricQueryStatementBuilder) buildPipelineStatement(
 			query.Aggregations[0].TimeAggregation = metrictypes.TimeAggregationIncrease
 		}
 		query.Aggregations[0].SpaceAggregation = metrictypes.SpaceAggregationSum
+
+		// check for origTimeAgg's and origSpaceAgg's validity
+		if origTimeAgg.IsZero() || !origTimeAgg.IsValid() {
+			return nil, errors.Newf(
+				errors.TypeInvalidInput,
+				errors.CodeInvalidInput,
+				"invalid time aggregation, should be one of the following: [`rate`, `increase`]",
+			)
+		}
+
+		if origSpaceAgg.IsZero() || !origSpaceAgg.IsValid() {
+			return nil, errors.Newf(
+				errors.TypeInvalidInput,
+				errors.CodeInvalidInput,
+				"invalid space aggregation, should be one of the following: [`count`, `p50`, `p75`, `p90`, `p95`, `p99`]",
+			)
+		}
 	}
 
 	var timeSeriesCTE string
@@ -523,11 +540,26 @@ func (b *MetricQueryStatementBuilder) buildSpatialAggregationCTE(
 	query qbtypes.QueryBuilderQuery[qbtypes.MetricAggregation],
 	_ map[string][]*telemetrytypes.TelemetryFieldKey,
 ) (string, []any, error) {
-	if query.Aggregations[0].SpaceAggregation.IsZero() {
+	if query.Aggregations[0].SpaceAggregation.IsZero() || !query.Aggregations[0].SpaceAggregation.IsValid() {
+		if query.Aggregations[0].Type.IsPercentileSpaceAggregationAllowed() {
+			return "", nil, errors.Newf(
+				errors.TypeInvalidInput,
+				errors.CodeInvalidInput,
+				"invalid space aggregation, should be one of the following: [`sum`, `avg`, `min`, `max`, `count`, `p50`, `p75`, `p90`, `p95`, `p99`]",
+			)
+		} else {
+			return "", nil, errors.Newf(
+				errors.TypeInvalidInput,
+				errors.CodeInvalidInput,
+				"invalid space aggregation, should be one of the following: [`sum`, `avg`, `min`, `max`, `count`]",
+			)
+		}
+	}
+	if query.Aggregations[0].SpaceAggregation.IsPercentile() && !query.Aggregations[0].Type.IsPercentileSpaceAggregationAllowed() {
 		return "", nil, errors.Newf(
 			errors.TypeInvalidInput,
 			errors.CodeInvalidInput,
-			"invalid space aggregation, should be one of the following: [`sum`, `avg`, `min`, `max`, `count`, `p50`, `p75`, `p90`, `p95`, `p99`]",
+			"percentile based aggregations are invalid for this metric, should be one of the following: [`sum`, `avg`, `min`, `max`, `count`]",
 		)
 	}
 	sb := sqlbuilder.NewSelectBuilder()

pkg/telemetrymetrics/stmt_builder_test.go

@@ -122,7 +122,7 @@ func TestStatementBuilder(t *testing.T) {
 			expectedErr: nil,
 		},
 		{
-			name:        "test_histogram_percentile",
+			name:        "test_histogram_percentile1",
 			requestType: qbtypes.RequestTypeTimeSeries,
 			query: qbtypes.QueryBuilderQuery[qbtypes.MetricAggregation]{
 				Signal:       telemetrytypes.SignalMetrics,
@@ -132,6 +132,7 @@ func TestStatementBuilder(t *testing.T) {
 						MetricName:       "signoz_latency",
 						Type:             metrictypes.HistogramType,
 						Temporality:      metrictypes.Delta,
+						TimeAggregation:  metrictypes.TimeAggregationRate,
 						SpaceAggregation: metrictypes.SpaceAggregationPercentile95,
 					},
 				},
@@ -187,7 +188,7 @@ func TestStatementBuilder(t *testing.T) {
 			expectedErr: nil,
 		},
 		{
-			name:        "test_histogram_percentile",
+			name:        "test_histogram_percentile2",
 			requestType: qbtypes.RequestTypeTimeSeries,
 			query: qbtypes.QueryBuilderQuery[qbtypes.MetricAggregation]{
 				Signal:       telemetrytypes.SignalMetrics,
@@ -197,6 +198,7 @@ func TestStatementBuilder(t *testing.T) {
 						MetricName:       "http_server_duration_bucket",
 						Type:             metrictypes.HistogramType,
 						Temporality:      metrictypes.Cumulative,
+						TimeAggregation:  metrictypes.TimeAggregationRate,
 						SpaceAggregation: metrictypes.SpaceAggregationPercentile95,
 					},
 				},
@@ -211,7 +213,7 @@ func TestStatementBuilder(t *testing.T) {
 			},
 			expected: qbtypes.Statement{
 				Query: "WITH __temporal_aggregation_cte AS (SELECT ts, `service.name`, `le`, multiIf(row_number() OVER rate_window = 1, nan, (per_series_value - lagInFrame(per_series_value, 1) OVER rate_window) < 0, per_series_value / (ts - lagInFrame(ts, 1) OVER rate_window), (per_series_value - lagInFrame(per_series_value, 1) OVER rate_window) / (ts - lagInFrame(ts, 1) OVER rate_window)) AS per_series_value FROM (SELECT fingerprint, toStartOfInterval(toDateTime(intDiv(unix_milli, 1000)), toIntervalSecond(30)) AS ts, `service.name`, `le`, max(value) AS per_series_value FROM signoz_metrics.distributed_samples_v4 AS points INNER JOIN (SELECT fingerprint, JSONExtractString(labels, 'service.name') AS `service.name`, JSONExtractString(labels, 'le') AS `le` FROM signoz_metrics.time_series_v4_6hrs WHERE metric_name IN (?) AND unix_milli >= ? AND unix_milli <= ? AND LOWER(temporality) LIKE LOWER(?) AND __normalized = ? GROUP BY fingerprint, `service.name`, `le`) AS filtered_time_series ON points.fingerprint = filtered_time_series.fingerprint WHERE metric_name IN (?) AND unix_milli >= ? AND unix_milli < ? GROUP BY fingerprint, ts, `service.name`, `le` ORDER BY fingerprint, ts) WINDOW rate_window AS (PARTITION BY fingerprint ORDER BY fingerprint, ts)), __spatial_aggregation_cte AS (SELECT ts, `service.name`, `le`, sum(per_series_value) AS value FROM __temporal_aggregation_cte WHERE isNaN(per_series_value) = ? GROUP BY ts, `service.name`, `le`) SELECT ts, `service.name`, histogramQuantile(arrayMap(x -> toFloat64(x), groupArray(le)), groupArray(value), 0.950) AS value FROM __spatial_aggregation_cte GROUP BY `service.name`, ts ORDER BY `service.name`, ts",
-				Args:  []any{"http_server_duration_bucket", uint64(1747936800000), uint64(1747983420000), "cumulative", false, "http_server_duration_bucket", uint64(1747947390000), uint64(1747983420000), 0},
+				Args:  []any{"http_server_duration_bucket", uint64(1747936800000), uint64(1747983420000), "cumulative", false, "http_server_duration_bucket", uint64(1747947360000), uint64(1747983420000), 0},
 			},
 			expectedErr: nil,
 		},

pkg/telemetrystore/telemetrystorehook/instrumentation.go

@@ -5,7 +5,6 @@ import (
 
 	"github.com/SigNoz/signoz/pkg/factory"
 	"github.com/SigNoz/signoz/pkg/telemetrystore"
-	"github.com/SigNoz/signoz/pkg/types/ctxtypes"
 	"go.opentelemetry.io/otel/attribute"
 	"go.opentelemetry.io/otel/codes"
 	"go.opentelemetry.io/otel/metric"
@@ -36,14 +35,7 @@ func NewInstrumentation(ctx context.Context, providerSettings factory.ProviderSe
 }
 
 func (hook *instrumentation) BeforeQuery(ctx context.Context, event *telemetrystore.QueryEvent) context.Context {
-	ctx, span := hook.tracer.Start(ctx, "", trace.WithSpanKind(trace.SpanKindClient))
-
-	// add trace_id and span_id to the log_comment
-	comment := ctxtypes.CommentFromContext(ctx)
-	comment.Set("trace_id", span.SpanContext().TraceID().String())
-	comment.Set("span_id", span.SpanContext().SpanID().String())
-	ctx = ctxtypes.NewContextWithComment(ctx, comment)
-
+	ctx, _ = hook.tracer.Start(ctx, "", trace.WithSpanKind(trace.SpanKindClient))
 	return ctx
 }
 

pkg/types/metrictypes/metrictypes.go

@@ -3,6 +3,7 @@ package metrictypes
 import (
 	"database/sql/driver"
 	"fmt"
+	"slices"
 	"strings"
 
 	"github.com/SigNoz/signoz/pkg/errors"
@@ -135,6 +136,10 @@ func (t *Type) Scan(src interface{}) error {
 	return nil
 }
 
+func (t Type) IsPercentileSpaceAggregationAllowed() bool {
+	return t == HistogramType || t == ExpHistogramType || t == SummaryType
+}
+
 var (
 	GaugeType        = Type{valuer.NewString("gauge")}
 	SumType          = Type{valuer.NewString("sum")}
@@ -185,6 +190,10 @@ func (TimeAggregation) Enum() []any {
 	}
 }
 
+func (t TimeAggregation) IsValid() bool {
+	return slices.ContainsFunc(t.Enum(), func(v any) bool { return v == t })
+}
+
 type SpaceAggregation struct {
 	valuer.String
 }
@@ -218,6 +227,10 @@ func (SpaceAggregation) Enum() []any {
 	}
 }
 
+func (s SpaceAggregation) IsValid() bool {
+	return slices.ContainsFunc(s.Enum(), func(v any) bool { return v == s })
+}
+
 func (s SpaceAggregation) IsPercentile() bool {
 	return s == SpaceAggregationPercentile50 ||
 		s == SpaceAggregationPercentile75 ||

pkg/types/querybuildertypes/querybuildertypesv5/builder_query.go

@@ -2,7 +2,6 @@ package querybuildertypesv5
 
 import (
 	"fmt"
-	"time"
 
 	"github.com/SigNoz/signoz/pkg/types/metrictypes"
 	"github.com/SigNoz/signoz/pkg/types/telemetrytypes"
@@ -227,28 +226,3 @@ func CanShortCircuitDelta(metricAgg MetricAggregation) bool {
 
 	return false
 }
-
-func DurationBucket(fromMS, toMS uint64) string {
-	diff := time.Unix(0, int64(toMS)).Sub(time.Unix(0, int64(fromMS)))
-
-	buckets := []struct {
-		d time.Duration
-		l string
-	}{
-		{1 * time.Hour, "<1h"},
-		{6 * time.Hour, "<6h"},
-		{24 * time.Hour, "<24h"},
-		{3 * 24 * time.Hour, "<3D"},
-		{7 * 24 * time.Hour, "<1W"},
-		{14 * 24 * time.Hour, "<2W"},
-		{30 * 24 * time.Hour, "<1M"},
-	}
-
-	for _, b := range buckets {
-		if diff < b.d {
-			return b.l
-		}
-	}
-
-	return ">=1M"
-}