internal/telemetry: track tracer init time metric #1896

lievan · 2023-04-12T19:35:30Z

What does this PR do?

Implements TimeGauge telemetry function and Distributions telemetry event to track tracer init time via the dd.instrumentation_telemetry_data.tracers.tracer_init_time metric.

Motivation

Describe how to test/QA your changes

Start a sample app with tracing and turn on the DD_INSTRUMENTATION_TELEMETRY_DEBUG flag.

Search for dd.instrumentation_telemetry_data.tracers.tracer_init_time in the staging metrics explorer.

Query for service:tracer-telemetry-intake @payload_dog_tag.request_type:distributions in staging logs

Reviewer's Checklist

Changed code has unit tests for its functionality.
If this interacts with the agent in a new way, a system test has been added.

pr-commenter · 2023-04-12T19:41:45Z

Benchmarks

Comparing candidate commit 160e661 in PR branch evan.li/time-gauge-2 with baseline commit 361d193 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 18 metrics, 0 unstable metrics.

…, so don't call unless the message is gauranteed to be flushed

… into evan.li/time-gauge-2

lievan · 2023-04-14T21:13:25Z

internal/telemetry/client.go

+
+	// copy over requests so we can do the actual submission without holding
+	// the lock. Zero out the old stuff so we don't leak references
+	for i, r := range c.requests {


We have to populate the submissions slice before adding metrics events so that the sequence id's are in the right order. Sequence id represents the order in which telemetry events happened. newRequest increments the client's sequence id, and newRequest for metrics is only called inside the flush function.

system tests were failing because the submissions array had out-of-order sequence id's, for example:
[6, 1, 2, 3, 4, 5].
Telemetry system tests does not expect the events to arrive in a strict order, but it will assert that out-of-order events arrive within a certain timeframe of each other (0.3 seconds), and in this example event 6 was arriving way before event 5.

lievan · 2023-04-14T21:18:27Z

internal/telemetry/client_test.go

 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.Header.Get("DD-Telemetry-Request-Type") == string(RequestTypeAppClosing) {


We can't rely on app-closing to flush the metrics anymore, since app-closing is sent before metrics. Now we'll just hard code the expected metric messages and wait until we've hit that count.

ajgajg1134

A few questions and comments here and there, looking pretty good!

ajgajg1134 · 2023-04-17T15:51:34Z

internal/telemetry/client.go

@@ -144,6 +144,8 @@ type client struct {
 	// metrics are sent
 	metrics    map[Namespace]map[string]*metric
 	newMetrics bool
+	// flush interval incremented every time flush() is called
+	interval int


Is this used anywhere? It looks like it's always 0 in the tests?

It's not needed - I originally had this when I interpreted interval for Gauge to mean the flush interval - will remove it!

We can add interval back somewhere appropriate when we discover a good use case for Gauge and Rate type metrics.

internal/telemetry/client.go

internal/telemetry/client_test.go

internal/telemetry/message.go

internal/telemetry/telemetry.go

Co-authored-by: Andrew Glaude <andrew.glaude@datadoghq.com>

… into evan.li/time-gauge-2

Co-authored-by: Andrew Glaude <andrew.glaude@datadoghq.com>

… into evan.li/time-gauge-2

ajgajg1134

Almost there! just two small comments

ajgajg1134 · 2023-04-18T17:53:22Z

internal/telemetry/client_test.go

+		client.Record(NamespaceTracers, MetricKindDist, "soobar", 1, nil, false)
+		client.Record(NamespaceTracers, MetricKindDist, "soobar", 3, nil, false)
+		client.mu.Lock()
+		client.flush()


Doesn't stop trigger a flush? So is this needed?

I actually meant to remove client.stop() here, thanks for pointing this out!
I wanted to replace stop with a direct call to flush so this unit test doesn't rely on the functionality of stop. Implementation of stop may change in the future because of our conversation about only sending app-closing when SIGINT and SIGQUIT are intercepted.

internal/telemetry/message.go

ajgajg1134

Thanks for the detailed QA notes!

…ch as queuename tags contrib: upgrade labstack/echo/v4 from v4.2.0 to v4.9.0 (#1891) ci: fix flaky lint job (#1892) contrib/elasticsearch: use naming schema (#1897) ci: introduce golangci (#1898) appsec: suspicious request blocking (#1797) Co-authored-by: Julio Guerra <julio@datadog.com> ci/golangci-lint: skip google.golang.org/grpc.v12 (#1899) .github/workflows: run ASM and RC system-tests scenarios (#1900) contrib/hashicorp/vault: use naming schema (#1868) contrib/database/sql: add WithIgnoreQueryTypes option (#1823) Co-authored-by: Zarir Hamza <zarir.hamza@datadoghq.com> Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com> contrib/database/sql: use naming schema (#1895) internal/appsec: add server.request.method address (#1893) Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com> Co-authored-by: François Mazeau <francois.mazeau@datadoghq.com> internal/appsec/dyngo: atomic instrumentation swapping (#1873) Co-authored-by: François Mazeau <francois.mazeau@datadoghq.com> go.mod: datadog-agent/pkg/remoteconfig/state 7.45.0-rc.1 (#1902) internal/version: bump to v1.51.0 (#1912) ddtrace/tracer: don't set empty tracestate propagation tag (#1910) go.mod: github.com/DataDog/datadog-agent/pkg/obfuscate 7.45.0-rc.1 (#1916) appsec: add blocking SDK body operation (#1901) * Modifying the appsec api: adding appsec.MonitorParsedHTTPBody an error as return value * Adding a call to the WAF to check for security event synchronously with a call to appsec.MonitorParsedHTTPBody on the body passed as parameter * Removing the call to the WAF done on the body an the end of a request because we moved it. * Refactoring the waf addresses storage and access Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com> ddtrace/{opentelemetry,opentracer}: add telemetry (#1909) internal/appsec: fix user ID event detection (#1918) internal/telemetry: track tracer init time metric (#1896) Co-authored-by: Andrew Glaude <andrew.glaude@datadoghq.com> internal/appsec/remoteconfig: fix rules overrides (#1921) contrib/mongodb: use naming schema (#1908) contrib/syndtr/goleveldb/leveldb: use naming schema (#1914) contrib/tidwall/buntdb: use naming schema (#1913) internal/appsec: do not ignore the appsec events rate limiter (#1927) remoteconfig: remove empty products and don't override appsec rules data (#1925) contrib/kafka: refactor tests (#1907) contrib/google.golang.org/grpc: use naming schema (#1919) contrib/twitchtv/twirp: use naming schema (#1920) contrib/http: use naming schema (#1929) ddtrace/tracer: reset decision maker during fallback behavior of w3c header extraction (#1933) contrib/cassandra: use naming schema (#1911) Co-authored-by: Diana Shevchenko <40775148+dianashevchenko@users.noreply.github.com> contrib/redis: use naming schema (#1906) Co-authored-by: Andrew Glaude <andrew.glaude@datadoghq.com> ci/system-tests: more scenarios with parallel jobs (#1938) ci: update linter job and add bodyclose (#1942) contrib/redis/go-redis.v9: support v9 (#1730) Add support for new go-redis version v9. It does 2 things: Copy existing version 8 files to a new path, /redis/go-redis.v9. Make changes to support version 9. Fixes #1710 format and rerun go tidy get rid of prints add topLevelRegion assertions remove confusing named return values and todo comment ddtrace/tracer: ensure access to trace tags is concurrency-safe (#1948) Spancontext marshaling was accessing tracer internal structures without a lock, resulting in a data race and panic. This commit adds a few methods to trace to allow safe access to the tags and propagatingTags members of trace to the marshaling code. Fixes #1944 ddtrace/tracer: mark context updated when SetUser is called (#1949) Fixes a minor logic mistake when setting a user on a span lint and add default switch case refactor resourceNameKey and value assignments restructure functions to be left aligned use internal logger, be less verbose with function names go back to normal switch type and format Set keyTraceID128 on first span in the chunk only (#1946) go.mod: upgrade go-libddwaf to v1.2.0 (#1953) Co-authored-by: Julio Guerra <julio@datadog.com> contrib/database/sql: fix bug where options were always overwritten by register options (#1904) Co-authored-by: Diana Shevchenko <40775148+dianashevchenko@users.noreply.github.com> ci/smoke-tests: update the go.sum file after go get -u (#1957) contrib/net/http: don't set empty string values as span tags (#1956) Do not set span fields when they are not configured so the tracer can put the defaults in. use normal string then derefence rever go.mod and go.sum changes contrib/internal/httptrace: remove naming schema from init (#1960) contrib/graphql: use naming schema (#1926) internal/telemetry: trim the dependencies version prefix v (#1963) contrib/aws: use naming schema (#1931) contrib/cloud.google.com/go/pubsub.v1: use naming schema (#1937) go mod tidy lint and fix test

lievan added 2 commits April 11, 2023 12:34

tracer start time telemetry metric

b2fba61

include required interval field, but keep it 0

2356bb1

lievan added this to the v1.50.0 milestone Apr 12, 2023

lievan requested a review from a team as a code owner April 12, 2023 19:35

lievan requested a review from a team April 12, 2023 19:35

lint:

d57cf67

lievan marked this pull request as draft April 12, 2023 20:21

lievan and others added 7 commits April 13, 2023 17:23

add support for Distributions message type

28f6c45

Merge branch 'main' into evan.li/time-gauge-2

fff9c73

Merge branch 'main' into evan.li/time-gauge-2

85d6f15

fix non consequtive seq ids -- newRequest increments seq id each time…

be60bb2

…, so don't call unless the message is gauranteed to be flushed

Merge branch 'evan.li/time-gauge-2' of github.com:DataDog/dd-trace-go…

7ec06c2

… into evan.li/time-gauge-2

fix seq id order in requests arr

02abee5

fix test concurrent client

5a3905e

lievan commented Apr 14, 2023

View reviewed changes

Merge branch 'main' into evan.li/time-gauge-2

1f622e4

lievan marked this pull request as ready for review April 14, 2023 21:18

ajgajg1134 requested changes Apr 17, 2023

View reviewed changes

lievan and others added 10 commits April 18, 2023 09:55

Update internal/telemetry/client.go

ad1a40f

Co-authored-by: Andrew Glaude <andrew.glaude@datadoghq.com>

Update internal/telemetry/client.go

e7e319d

Co-authored-by: Andrew Glaude <andrew.glaude@datadoghq.com>

Update internal/telemetry/client.go

e6f9c1f

Co-authored-by: Andrew Glaude <andrew.glaude@datadoghq.com>

Update internal/telemetry/client_test.go

7e60a1c

Co-authored-by: Andrew Glaude <andrew.glaude@datadoghq.com>

refactor code

a89bc4b

Merge branch 'evan.li/time-gauge-2' of github.com:DataDog/dd-trace-go…

fdab93b

… into evan.li/time-gauge-2

Update internal/telemetry/message.go

0e4bb32

Co-authored-by: Andrew Glaude <andrew.glaude@datadoghq.com>

Merge branch 'evan.li/time-gauge-2' of github.com:DataDog/dd-trace-go…

95fd5d9

… into evan.li/time-gauge-2

merge w main

5879375

clarify comment on record

26c3d80

lievan changed the base branch from main to shared/128-bit April 18, 2023 14:36

lievan requested review from a team as code owners April 18, 2023 14:36

lievan changed the base branch from shared/128-bit to main April 18, 2023 14:36

lievan removed request for a team April 18, 2023 14:37

lievan added 2 commits April 18, 2023 12:47

fix distributions metric bug, add test case

1af77bc

fix lint

556ea67

ajgajg1134 requested changes Apr 18, 2023

View reviewed changes

lievan and others added 3 commits April 18, 2023 14:25

spelling

28877b5

remove call to stop

da408b1

Merge branch 'main' into evan.li/time-gauge-2

160e661

ajgajg1134 approved these changes Apr 18, 2023

View reviewed changes

lievan merged commit 58ba73d into main Apr 18, 2023

lievan deleted the evan.li/time-gauge-2 branch April 18, 2023 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/telemetry: track tracer init time metric #1896

internal/telemetry: track tracer init time metric #1896

lievan commented Apr 12, 2023 •

edited

Loading

pr-commenter bot commented Apr 12, 2023 •

edited

Loading

lievan Apr 14, 2023

lievan Apr 14, 2023

ajgajg1134 left a comment

ajgajg1134 Apr 17, 2023

lievan Apr 18, 2023

lievan Apr 18, 2023

ajgajg1134 left a comment

ajgajg1134 Apr 18, 2023

lievan Apr 18, 2023

ajgajg1134 left a comment

		server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		if r.Header.Get("DD-Telemetry-Request-Type") == string(RequestTypeAppClosing) {

internal/telemetry: track tracer init time metric #1896

internal/telemetry: track tracer init time metric #1896

Conversation

lievan commented Apr 12, 2023 • edited Loading

What does this PR do?

Motivation

Describe how to test/QA your changes

Reviewer's Checklist

pr-commenter bot commented Apr 12, 2023 • edited Loading

Benchmarks

lievan Apr 14, 2023

Choose a reason for hiding this comment

lievan Apr 14, 2023

Choose a reason for hiding this comment

ajgajg1134 left a comment

Choose a reason for hiding this comment

ajgajg1134 Apr 17, 2023

Choose a reason for hiding this comment

lievan Apr 18, 2023

Choose a reason for hiding this comment

lievan Apr 18, 2023

Choose a reason for hiding this comment

ajgajg1134 left a comment

Choose a reason for hiding this comment

ajgajg1134 Apr 18, 2023

Choose a reason for hiding this comment

lievan Apr 18, 2023

Choose a reason for hiding this comment

ajgajg1134 left a comment

Choose a reason for hiding this comment

lievan commented Apr 12, 2023 •

edited

Loading

pr-commenter bot commented Apr 12, 2023 •

edited

Loading