feat!: replace the influx trace client with the local trace client #1279
Conversation
evan-forbes
added
the
WS: Big Blonks 🔭
evan-forbes
requested review from
cmwaters,
staheri14,
rach-id,
ninabarbakadze and
rootulp
as code owners
|
Are we planning on deprecating support for influx in favour of this? |
evan-forbes
changed the title
|
|
pkg/trace/local_client.go
Outdated
| eventJSON, err := json.Marshal(event) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshal event: %v", err) | ||
| } |
There was a problem hiding this comment.
thought about using csv, as that should be faster and create significantly smaller files, however the std lib csv is kinda meh tbh and I didn't want to introduce a new dep. if we're ok with using https://github.com/jszwec/csvutil , then I'm totally down.
|
Could you please include some guidelines on how to utilize the new tracing feature? Alternatively, if this feature is not yet operational within the current PR, an overview of the modifications made would be highly appreciated. |
true good point, however for 100MB block parts we can send and receive a combined 50_000 block parts. Still definitely doable, but noticable. We've observed in the past, for example in #1145, that we can see different problematic patterns at different heights. We might be able to recreate those scenarios in specific tests, however I think having the ability to trace everything gives us the most freedom in finding those weird edge cases |
## Description Adds the pull based portion of the file server along with a simple client.
Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>
…org/celestia-core into evan/local-trace-client
| if cfg.TraceBufferSize < 0 { | ||
| return fmt.Errorf("trace buffer size must be greater than 0") | ||
| } |
There was a problem hiding this comment.
Can it be 0? Do we not want <=
There was a problem hiding this comment.
technically speaking, 0 could work although ofc that's a bad idea. The reasoning for switching here was so that a value of 0 won't fail for when the default value isn't used in some tests, and that I wasn't sure where to stop. While 0 is not reccomended, neither is 1. combining those two things resulted in just switching this to 0
| // V1VersionFieldValue is a tracing field value for the version of | ||
| // the mempool. This value is used by the "version" field key. | ||
| V1VersionFieldValue = "v1" | ||
| // MemPoolTx describes the schema for the "mempool_tx" table. |
There was a problem hiding this comment.
[nit]
| // MemPoolTx describes the schema for the "mempool_tx" table. | |
| // MempoolTx describes the schema for the "mempool_tx" table. |
There was a problem hiding this comment.
I'll grab this in a followup, thanks
| # The list of tables that are updated when tracing. All available tables and | ||
| # their schema can be found in the pkg/trace/schema package. It is represented as a | ||
| # comma separate string. For example: "consensus_round_state,mempool_tx". | ||
| tracing_tables = "consensus_round_state,mempool_tx" |
There was a problem hiding this comment.
[nit] Based on my testing, this field automatically gets populated with the names of all available tables hence, I suppose there is no need for modifications unless there's a specific desire to exclude certain tables from the tracing process. If that's the case, it would indeed be helpful to clarify that it is possible to selectively skip tracing specific tables.
There was a problem hiding this comment.
I think I'm having some author blindness here 😅, so I'll defer to you on this. are you refering to the readme, or do you think the first sentence here doesn't describe that? if its the first sentence, do you think
The list of tables that are updated when tracing. Tables not included below will not be traced.
works better?
|
also, thanks @staheri14 for manually trying out the code!!! 😃 |
manual of backport of #1279
Description
eventually closes #1278
super breaking for the config