feat!: replace the influx tracer with a local one #1290

evan-forbes · 2024-04-05T23:12:10Z

manual of backport of #1279

refactor!: convert to using a tracer interface feat: add the local client refactor!: fix the schema package feat: decode with types feat!: add ability to change the tracer in the config chore!: remove support for influx chore!: finish purging all things influx chore: remove unnedded e2e setup chore: remove server to reduce changes in this PR chore: linter chore: linter fix: just use the noop for the default tracer to avoid annoying failures feat: concurrently read and write using new a new file and buffer chore: redelete the fileserver fix: give up on being fancy with no mutexes and optimize later docs: add readme docs: update docs in toml fix: go vuln fix: cat mempool tracing chore: go mod tidy reafactor!: rename everything refactor!: convert to using a tracer interface feat: add the local client refactor!: fix the schema package feat: decode with types feat!: add ability to change the tracer in the config chore!: remove support for influx chore!: finish purging all things influx chore: remove unnedded e2e setup chore: remove server to reduce changes in this PR chore: linter chore: linter fix: just use the noop for the default tracer to avoid annoying failures feat: concurrently read and write using new a new file and buffer chore: redelete the fileserver fix: give up on being fancy with no mutexes and optimize later docs: add readme docs: update docs in toml fix: go vuln feat: add the pull based fileserver fix: remove second server in test chore: linter chore: rename chore: linter chore: cleanup old docs feat: add the ability to push traces to an s3 bucket fix: rebase

evan-forbes · 2024-04-06T16:26:17Z

wat

pkg/trace/fileserver.go

cmwaters

I think we should be wary that we're using v0.34.x-celestia for both v1 and v2 which could be breaking in some ways i.e. with the config and could thus make it difficult to release patches if they were needed

rootulp

[question] why do we want to backport a config breaking change to v0.34.x? How do we intend on releasing this change? Will it be v2.0.0-tm-v0.34.29?

If that's the case, what's our current branch management strategy in this repo because I think this backport conflicts with my understanding:

v0.34.x-celestia was originally based on tendermint's v0.34.x release branch but now it receives patches from the CometBFT v0.34.x release branch. This branch also contains Celestia-specific changes. Future v1.x.0-tm-v0.34.x releases of this repo will be based on this branch.

Ref: https://github.com/celestiaorg/celestia-core?tab=readme-ov-file#branches

rootulp · 2024-04-12T00:30:54Z

config/config.go

-	// InfluxURL is the influxdb url.
-	InfluxURL string `mapstructure:"influx_url"`
+	// TracePushConfig is the relative path of the push config. This second
+	// config contains credentials for where and how often to.


?

Suggested change

// config contains credentials for where and how often to.

// config contains credentials for where and how often to push.

rootulp · 2024-04-12T00:33:18Z

config/config.go

+	// TraceType is the type of tracer used. Options are "local" and "noop".
+	TraceType string `mapstructure:"trace_type"`


[question] why not continue supporting the existing config + pushing to InfluxDB and add the local tracer as a strictly new feature + new config? I think that could make this PR non-breaking.

we have to have a fix for #1141 , which is already breaking, but also keeping the old tracer was pretty burdensome as we had to have a ton of consts and enforce the schema in a really weird way. Here, we're just using a struct, but if we keep the influx, we have to keep two separate mechanisms for enforcing the schema

rootulp · 2024-04-12T00:34:16Z

config/config.go

-	// InfluxBucket is the influxdb bucket.
-	InfluxBucket string `mapstructure:"influx_bucket"`
+	// TraceBufferSize is the number of traces to write in a single batch.
+	TraceBufferSize int `mapstructure:"trace_push_batch_size"`


[nit] why don't the field name and tag match?

Suggested change

TraceBufferSize int `mapstructure:"trace_push_batch_size"`

TraceBufferSize int `mapstructure:"trace_buffer_size"`

just forgot to change. fixing upstream and moving down

rootulp · 2024-04-12T00:35:29Z

config/config.go

 		return nil
 	}
-	if cfg.InfluxToken == "" {
+	if cfg.TracePullAddress == "" {
 		return fmt.Errorf("token is required")


[blocking] this error looks misleading given the field rename

Suggested change

return fmt.Errorf("token is required")

return fmt.Errorf("trace pull address is required")

On second thought, is this field optional or required? I suspect it's optional in which case the whole conditional can be deleted.

Copying code from elsewhere in this PR:

# The tracer pull address specifies which address will be used for pull based # event collection. If empty, the pull based server will not be started.

seems optional so we shouldn't error here

rootulp · 2024-04-12T00:36:43Z

config/config.go

+	if cfg.TraceType == "" {
 		return fmt.Errorf("org is required")


[blocking] this error is misleading

Suggested change

if cfg.TraceType == "" {

return fmt.Errorf("org is required")

if cfg.TraceType == "" {

return fmt.Errorf("TraceType is required")

fixing upstream and moving down

rootulp · 2024-04-12T00:37:40Z

config/toml.go

-# collection. If empty, remote event collection is disabled.
-influx_url = "{{ .Instrumentation.InfluxURL }}"
+# TracePushConfig is the relative path of the push config.
+# This second config contains credentials for where and how often to


idk what "second" means here? What was the first config?

the second config is a json that contains creds to accessing an s3 bucket

rootulp · 2024-04-12T00:41:36Z

pkg/trace/README.md

+Trace data will now be stored to the `.celestia-app/data/traces` directory, and
+save the file to the specified directory in the `table_name.jsonl` format.


grammar looks off

Suggested change

Trace data will now be stored to the `.celestia-app/data/traces` directory, and

save the file to the specified directory in the `table_name.jsonl` format.

Trace data will now be stored to the `.celestia-app/data/traces` directory, and

files will be saved in the `table_name.jsonl` format.

fixing upstream and moving down

rootulp · 2024-04-12T00:44:04Z

pkg/trace/doc.go

-make && ./build/runner -f ./networks/ci.toml --influxdb-url=http://your-influx-ip:8086/ --influxdb-token="your-token"
-```
-*/
+/**/


Suggested change

/**/

rootulp · 2024-04-12T00:51:07Z

Also sorry I didn't review the original PR. It's okay if you don't want to address my feedback on this PR because the divergence between the PR that already merged. I would still like to understand the release strategy for this breaking change.

evan-forbes · 2024-04-12T05:28:29Z

I would still like to understand the release strategy for this breaking change.

this is a good point, and we should document this. in the past, we've briefly discussed using a migration script. In a similar vein to a migration script, if we know what the error is, then we can automatically fix the config and log it should that error occur.

we will address this in #1299

rootulp · 2024-04-15T15:47:01Z

During a 1:1 I think we decided to make the config change strictly additive and non-breaking so that we don't have to write a config migrator. If that's the case, do we still want this PR reviewed as-is?

evan-forbes · 2024-04-16T23:22:32Z

going to merge this, as I just tested and this is no longer breaking the config now that we've renamed everything. its still a super breaking change in that we've technically replaced an entire feature with something that is completely different, but we don't have to complete #1299

rootulp · 2024-04-17T00:12:53Z

@evan-forbes why did we merge this? As far as I can tell, it's a breaking change and it targeted a release branch.

evan-forbes self-assigned this Apr 5, 2024

evan-forbes requested review from cmwaters, staheri14, rach-id, ninabarbakadze and rootulp as code owners April 5, 2024 23:12

evan-forbes removed request for rootulp, rach-id and ninabarbakadze April 5, 2024 23:15

chore: linter exceptions

b012bf4

rootulp reviewed Apr 10, 2024

View reviewed changes

pkg/trace/fileserver.go Outdated Show resolved Hide resolved

chore: disable depguard for now

7f4d81d

rootulp requested review from rootulp and removed request for rootulp April 10, 2024 17:12

cmwaters approved these changes Apr 11, 2024

View reviewed changes

rootulp reviewed Apr 12, 2024

View reviewed changes

evan-forbes mentioned this pull request Apr 12, 2024

Implement a migration script for the breaking config in v0.34.x #1299

Closed

rootulp self-requested a review April 15, 2024 15:45

staheri14 approved these changes Apr 15, 2024

View reviewed changes

evan-forbes enabled auto-merge (squash) April 16, 2024 15:53

evan-forbes disabled auto-merge April 16, 2024 15:53

rootulp mentioned this pull request Apr 16, 2024

feat!: increase MaxBlockSizeBytes to 128 MiB (backport #1289) #1303

Closed

evan-forbes added the backport label Apr 16, 2024

evan-forbes enabled auto-merge (squash) April 16, 2024 23:22

Merge branch 'v0.34.x-celestia' into evan/tracer-patch

abd7c3b

evan-forbes requested a review from a team as a code owner April 16, 2024 23:22

evan-forbes requested review from ninabarbakadze and removed request for a team April 16, 2024 23:22

evan-forbes merged commit d3ff8e7 into v0.34.x-celestia Apr 16, 2024
21 checks passed

evan-forbes deleted the evan/tracer-patch branch April 16, 2024 23:28

rootulp mentioned this pull request May 9, 2024

chore(deps): bump core version to v1.36.0-tm-v0.34.29 (backport #3452) celestiaorg/celestia-app#3455

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: replace the influx tracer with a local one #1290

feat!: replace the influx tracer with a local one #1290

evan-forbes commented Apr 5, 2024 •

edited

Loading

evan-forbes commented Apr 6, 2024

cmwaters left a comment

rootulp left a comment

rootulp Apr 12, 2024

rootulp Apr 12, 2024

evan-forbes Apr 12, 2024

rootulp Apr 12, 2024

evan-forbes Apr 12, 2024 •

edited

Loading

rootulp Apr 12, 2024 •

edited

Loading

rootulp Apr 12, 2024

rootulp Apr 12, 2024 •

edited

Loading

evan-forbes Apr 12, 2024

rootulp Apr 12, 2024

evan-forbes Apr 12, 2024

rootulp Apr 12, 2024

evan-forbes Apr 12, 2024

rootulp Apr 12, 2024

rootulp commented Apr 12, 2024

evan-forbes commented Apr 12, 2024 •

edited

Loading

rootulp commented Apr 15, 2024

evan-forbes commented Apr 16, 2024

rootulp commented Apr 17, 2024 •

edited

Loading

	// config contains credentials for where and how often to.
	// config contains credentials for where and how often to push.

		// TraceType is the type of tracer used. Options are "local" and "noop".
		TraceType string `mapstructure:"trace_type"`

	TraceBufferSize int `mapstructure:"trace_push_batch_size"`
	TraceBufferSize int `mapstructure:"trace_buffer_size"`

	return fmt.Errorf("token is required")
	return fmt.Errorf("trace pull address is required")

		if cfg.TraceType == "" {
		return fmt.Errorf("org is required")

		Trace data will now be stored to the `.celestia-app/data/traces` directory, and
		save the file to the specified directory in the `table_name.jsonl` format.

feat!: replace the influx tracer with a local one #1290

feat!: replace the influx tracer with a local one #1290

Conversation

evan-forbes commented Apr 5, 2024 • edited Loading

evan-forbes commented Apr 6, 2024

cmwaters left a comment

Choose a reason for hiding this comment

rootulp left a 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

evan-forbes Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

rootulp Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootulp Apr 12, 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

rootulp commented Apr 12, 2024

evan-forbes commented Apr 12, 2024 • edited Loading

rootulp commented Apr 15, 2024

evan-forbes commented Apr 16, 2024

rootulp commented Apr 17, 2024 • edited Loading

evan-forbes commented Apr 5, 2024 •

edited

Loading

evan-forbes Apr 12, 2024 •

edited

Loading

rootulp Apr 12, 2024 •

edited

Loading

rootulp Apr 12, 2024 •

edited

Loading

evan-forbes commented Apr 12, 2024 •

edited

Loading

rootulp commented Apr 17, 2024 •

edited

Loading