Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(codecs) csv documentation defaults as ascii_char #20498

Merged
merged 4 commits into from
Jul 17, 2024

Conversation

scMarkus
Copy link
Contributor

work in progress PR to work out how to best solve #18669.

current state has been described here #18669 (comment)

@tobz woudl you suggest looking into the conversion between json schema file and cue documentation? if so may I ask you for a hint on where best to start?

@tobz
Copy link
Contributor

tobz commented May 21, 2024

@scMarkus You'll need to run make generate-component-docs to update the component docs that are affected by this change.

@scMarkus scMarkus requested review from a team as code owners June 25, 2024 11:34
@github-actions github-actions bot added domain: topology Anything related to Vector's topology code domain: sources Anything related to the Vector's sources domain: transforms Anything related to Vector's transform components domain: sinks Anything related to the Vector's sinks domain: ci Anything related to Vector's CI environment domain: releasing Anything related to releasing Vector domain: codecs Anything related to Vector's codecs (encoding/decoding) domain: external docs Anything related to Vector's external, public documentation domain: core Anything related to core crates i.e. vector-core, core-common, etc domain: vdev Anything related to the vdev tooling labels Jun 25, 2024
@scMarkus scMarkus force-pushed the fix_documentation_typing branch from d406b36 to d49d239 Compare June 25, 2024 11:39
@github-actions github-actions bot removed domain: topology Anything related to Vector's topology code domain: sources Anything related to the Vector's sources domain: transforms Anything related to Vector's transform components domain: sinks Anything related to the Vector's sinks domain: codecs Anything related to Vector's codecs (encoding/decoding) domain: core Anything related to core crates i.e. vector-core, core-common, etc domain: vdev Anything related to the vdev tooling labels Jun 25, 2024
@scMarkus
Copy link
Contributor Author

@tobz sorry for taking so long. I am now able to focus on this issue again.

From what I understand the rust specific part of adding meta data to a type by using mcros like #[configurable(metadata(docs::type_override = "ascii_char"))] seams to work reasonable. the generated json output does contain all of the needed information. What I think is missing, would be a correct handling of such information in the ruby script generating cue files. I have added a naive code branch handling my specific case. Pleas have a look at d49d239. The output does look like what I would expect in the cue files but I have not validated it in any rendered form since I would not know how.

I am quite certain that this is not the final solution we want to have so I am curious about your thoughts @tobz.

Copy link
Contributor

@drichards-87 drichards-87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some small suggestions from Docs and approved the PR.

@scMarkus scMarkus force-pushed the fix_documentation_typing branch from 933742f to 1add8a0 Compare June 25, 2024 17:46
@github-actions github-actions bot removed the domain: releasing Anything related to releasing Vector label Jun 25, 2024
@tobz
Copy link
Contributor

tobz commented Jun 27, 2024

@scMarkus Nice, the changes look good to me.

I thought we used to have a CI job that would publish a preview version of the docs based on the PR branch, but it seems like not... let me figure out the best way to preview the fully-generated docs and get back to you.

@tobz tobz added the no-changelog Changes in this PR do not need user-facing explanations in the release changelog label Jun 27, 2024
@tobz
Copy link
Contributor

tobz commented Jun 27, 2024

So, looking at the HTML template that gets used to render this part of the configuration docs, I'm pretty confident this should just work as-is.

It will end up looking like this:

Screenshot 2024-06-27 at 10 52 28 AM

While ascii_char is maybe a little weird, I don't think we need to worry about that here. The end result it's going to properly show a comma (or whatever the character is) and that seems like a win to me.

@tobz tobz enabled auto-merge June 27, 2024 14:54
auto-merge was automatically disabled June 27, 2024 20:28

Head branch was pushed to by a user without write access

@scMarkus scMarkus force-pushed the fix_documentation_typing branch 2 times, most recently from 65f3e5e to 24fc1a7 Compare June 27, 2024 20:47
@scMarkus
Copy link
Contributor Author

I have added the other two configuration parameters to our new ascii_char handling and added a note in the ruby script that the new behavior is meant for u8 data type only. From my point of view this would solve the issue as described in #18669.

Copy link
Contributor

@tobz tobz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one small suggested changed in the doc comment for the generation script.

scripts/generate-component-docs.rb Show resolved Hide resolved
@scMarkus
Copy link
Contributor Author

scMarkus commented Jul 2, 2024

In regards to the latest failed test run. if I understand the output correctly, there is an issue with make check-docs. Running this command locally does not find such errors. Pleas let me know if I can change anything to make the tests pass.

local execution:

~/tools/vector $ make check-docs ENVIRONMENT=true CONTAINER_TOOL="podman"
Building the environment. (ENVIRONMENT_AUTOBUILD=true) This may take a few minutes...
podman build --quiet --tag docker.io/timberio/vector-dev:sha-3eadc96742a33754a5859203b58249f6a806972a --file scripts/environment/Dockerfile .
6a585b575749b9677fc886abdaeadb26a80751da2e29e2a7f1f764e2c81be7ef
Entering environment...
podman run --name vector-environment --rm --tty --init --interactive --env INSIDE_ENVIRONMENT=true --network host --mount type=bind,source=/home/markus/tools/vector,target=/git/vectordotdev/vector  --mount type=volume,source=vector-target,target=/git/vectordotdev/vector/target --mount type=volume,source=vector-cargo-cache,target=/root/.cargo --mount type=volume,source=vector-rustup-cache,target=/root/.rustup  docker.io/timberio/vector-dev:sha-3eadc96742a33754a5859203b58249f6a806972a cargo vdev check docs
Skipping cue files format validation - reserved for CI
Validating cue files correctness...
Success! The contents of the sources in the "./website/cue" directory are valid

@scMarkus scMarkus force-pushed the fix_documentation_typing branch from fe647da to 00a3f65 Compare July 3, 2024 13:40
@scMarkus
Copy link
Contributor Author

Thus far I feel positive about this pull request. @tobz you have approved it already as well. Is there anything left to do in your opinion?

@tobz
Copy link
Contributor

tobz commented Jul 16, 2024

Sorry, this fell off my radar.

I agree that this is ready to go. 👍🏻

@tobz tobz added this pull request to the merge queue Jul 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 16, 2024
Copy link

Regression Detector Results

Run ID: 355f70c7-e696-4fe9-8943-3a22e8f9803c Metrics dashboard

Baseline: 1579627
Comparison: 053474e

Performance changes are noted in the perf column of each table:

  • ✅ = significantly better comparison variant performance
  • ❌ = significantly worse comparison variant performance
  • ➖ = no significant change in performance

No significant changes in experiment optimization goals

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.

Experiments ignored for regressions

Regressions in experiments with settings containing erratic: true are ignored.

perf experiment goal Δ mean % Δ mean % CI links
file_to_blackhole egress throughput -1.97 [-8.94, +5.01]

Fine details of change detection per experiment

perf experiment goal Δ mean % Δ mean % CI links
syslog_log2metric_tag_cardinality_limit_blackhole ingress throughput +2.59 [+2.46, +2.72]
syslog_log2metric_humio_metrics ingress throughput +2.24 [+2.11, +2.38]
syslog_humio_logs ingress throughput +1.96 [+1.83, +2.09]
datadog_agent_remap_datadog_logs ingress throughput +1.62 [+1.39, +1.85]
splunk_hec_route_s3 ingress throughput +1.58 [+1.27, +1.90]
fluent_elasticsearch ingress throughput +0.98 [+0.48, +1.47]
http_text_to_http_json ingress throughput +0.96 [+0.84, +1.08]
http_elasticsearch ingress throughput +0.90 [+0.70, +1.10]
http_to_http_acks ingress throughput +0.85 [-0.49, +2.18]
syslog_splunk_hec_logs ingress throughput +0.79 [+0.69, +0.89]
datadog_agent_remap_blackhole ingress throughput +0.44 [+0.34, +0.54]
datadog_agent_remap_blackhole_acks ingress throughput +0.25 [+0.15, +0.34]
otlp_http_to_blackhole ingress throughput +0.01 [-0.17, +0.18]
splunk_hec_to_splunk_hec_logs_acks ingress throughput +0.00 [-0.09, +0.10]
splunk_hec_indexer_ack_blackhole ingress throughput -0.02 [-0.10, +0.06]
splunk_hec_to_splunk_hec_logs_noack ingress throughput -0.02 [-0.13, +0.08]
syslog_log2metric_splunk_hec_metrics ingress throughput -0.09 [-0.20, +0.03]
http_to_http_json ingress throughput -0.10 [-0.18, -0.01]
otlp_grpc_to_blackhole ingress throughput -0.16 [-0.28, -0.03]
http_to_http_noack ingress throughput -0.16 [-0.27, -0.05]
http_to_s3 ingress throughput -0.61 [-0.88, -0.35]
syslog_loki ingress throughput -1.43 [-1.50, -1.35]
syslog_regex_logs2metric_ddmetrics ingress throughput -1.50 [-1.63, -1.36]
datadog_agent_remap_datadog_logs_acks ingress throughput -1.80 [-1.97, -1.63]
file_to_blackhole egress throughput -1.97 [-8.94, +5.01]
socket_to_socket_blackhole ingress throughput -3.15 [-3.21, -3.09]

Explanation

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

  1. Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.

  2. Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.

  3. Its configuration does not mark it "erratic".

@jszwedko jszwedko added this pull request to the merge queue Jul 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 16, 2024
Copy link

Regression Detector Results

Run ID: 4a10494b-83b2-41e9-aa33-efd6452b6f0d Metrics dashboard

Baseline: 1579627
Comparison: a94050e

Performance changes are noted in the perf column of each table:

  • ✅ = significantly better comparison variant performance
  • ❌ = significantly worse comparison variant performance
  • ➖ = no significant change in performance

No significant changes in experiment optimization goals

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.

Experiments ignored for regressions

Regressions in experiments with settings containing erratic: true are ignored.

perf experiment goal Δ mean % Δ mean % CI links
file_to_blackhole egress throughput +0.97 [-5.73, +7.67]

Fine details of change detection per experiment

perf experiment goal Δ mean % Δ mean % CI links
datadog_agent_remap_blackhole ingress throughput +1.74 [+1.64, +1.84]
syslog_log2metric_humio_metrics ingress throughput +1.70 [+1.56, +1.84]
file_to_blackhole egress throughput +0.97 [-5.73, +7.67]
fluent_elasticsearch ingress throughput +0.81 [+0.31, +1.30]
splunk_hec_route_s3 ingress throughput +0.54 [+0.22, +0.85]
syslog_loki ingress throughput +0.38 [+0.29, +0.47]
syslog_regex_logs2metric_ddmetrics ingress throughput +0.31 [+0.19, +0.44]
syslog_splunk_hec_logs ingress throughput +0.15 [+0.05, +0.25]
syslog_log2metric_splunk_hec_metrics ingress throughput +0.04 [-0.06, +0.14]
splunk_hec_to_splunk_hec_logs_acks ingress throughput +0.00 [-0.10, +0.10]
splunk_hec_indexer_ack_blackhole ingress throughput +0.00 [-0.08, +0.08]
splunk_hec_to_splunk_hec_logs_noack ingress throughput -0.00 [-0.10, +0.10]
otlp_grpc_to_blackhole ingress throughput -0.02 [-0.14, +0.10]
http_to_http_json ingress throughput -0.13 [-0.21, -0.06]
http_to_http_noack ingress throughput -0.19 [-0.30, -0.08]
datadog_agent_remap_datadog_logs_acks ingress throughput -0.45 [-0.66, -0.25]
datadog_agent_remap_blackhole_acks ingress throughput -0.56 [-0.66, -0.45]
http_elasticsearch ingress throughput -0.67 [-0.87, -0.48]
http_text_to_http_json ingress throughput -0.68 [-0.81, -0.54]
http_to_s3 ingress throughput -1.31 [-1.59, -1.03]
otlp_http_to_blackhole ingress throughput -1.36 [-1.53, -1.18]
http_to_http_acks ingress throughput -1.83 [-3.14, -0.51]
syslog_humio_logs ingress throughput -1.85 [-1.97, -1.73]
socket_to_socket_blackhole ingress throughput -2.07 [-2.15, -1.98]
syslog_log2metric_tag_cardinality_limit_blackhole ingress throughput -2.07 [-2.16, -1.98]
datadog_agent_remap_datadog_logs ingress throughput -3.54 [-3.77, -3.32]

Explanation

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

  1. Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.

  2. Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.

  3. Its configuration does not mark it "erratic".

@scMarkus
Copy link
Contributor Author

No worries. Thanks for having this progressing :)

@jszwedko jszwedko added this pull request to the merge queue Jul 17, 2024
Copy link

Regression Detector Results

Run ID: f08b5ea8-d052-497c-bbac-21a583ae452c Metrics dashboard

Baseline: 7916ad5
Comparison: 5a10aa2

Performance changes are noted in the perf column of each table:

  • ✅ = significantly better comparison variant performance
  • ❌ = significantly worse comparison variant performance
  • ➖ = no significant change in performance

No significant changes in experiment optimization goals

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.

Experiments ignored for regressions

Regressions in experiments with settings containing erratic: true are ignored.

perf experiment goal Δ mean % Δ mean % CI links
file_to_blackhole egress throughput +15.40 [+7.78, +23.03]

Fine details of change detection per experiment

perf experiment goal Δ mean % Δ mean % CI links
file_to_blackhole egress throughput +15.40 [+7.78, +23.03]
http_to_http_acks ingress throughput +2.51 [+1.17, +3.86]
syslog_regex_logs2metric_ddmetrics ingress throughput +2.09 [+1.93, +2.25]
syslog_log2metric_splunk_hec_metrics ingress throughput +1.66 [+1.53, +1.78]
syslog_splunk_hec_logs ingress throughput +1.54 [+1.42, +1.66]
syslog_loki ingress throughput +0.85 [+0.77, +0.93]
splunk_hec_route_s3 ingress throughput +0.28 [-0.05, +0.61]
http_to_s3 ingress throughput +0.12 [-0.15, +0.39]
socket_to_socket_blackhole ingress throughput +0.08 [+0.03, +0.13]
splunk_hec_to_splunk_hec_logs_acks ingress throughput -0.01 [-0.13, +0.11]
fluent_elasticsearch ingress throughput -0.01 [-0.51, +0.49]
splunk_hec_to_splunk_hec_logs_noack ingress throughput -0.01 [-0.11, +0.09]
splunk_hec_indexer_ack_blackhole ingress throughput -0.03 [-0.12, +0.05]
http_to_http_noack ingress throughput -0.10 [-0.22, +0.02]
http_to_http_json ingress throughput -0.12 [-0.19, -0.04]
otlp_grpc_to_blackhole ingress throughput -0.15 [-0.27, -0.03]
syslog_log2metric_tag_cardinality_limit_blackhole ingress throughput -0.31 [-0.41, -0.20]
datadog_agent_remap_datadog_logs_acks ingress throughput -0.36 [-0.51, -0.20]
datadog_agent_remap_blackhole ingress throughput -0.45 [-0.55, -0.35]
otlp_http_to_blackhole ingress throughput -0.63 [-0.78, -0.48]
syslog_humio_logs ingress throughput -0.72 [-0.84, -0.59]
datadog_agent_remap_blackhole_acks ingress throughput -0.81 [-0.93, -0.70]
datadog_agent_remap_datadog_logs ingress throughput -1.23 [-1.44, -1.02]
http_text_to_http_json ingress throughput -1.67 [-1.83, -1.50]
syslog_log2metric_humio_metrics ingress throughput -1.77 [-1.93, -1.61]
http_elasticsearch ingress throughput -2.15 [-2.34, -1.96]

Explanation

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

  1. Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.

  2. Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.

  3. Its configuration does not mark it "erratic".

Merged via the queue into vectordotdev:master with commit 5a10aa2 Jul 17, 2024
49 checks passed
@scMarkus scMarkus deleted the fix_documentation_typing branch July 17, 2024 19:30
ym pushed a commit to ym/vector that referenced this pull request Aug 18, 2024
…0498)

* set type_override for ascii_char

* specially handle "docs::type_override" with "ascii_char" in generate-component-docs.rb

* add missing filds in csv.rs and coresponsing cue docs as well as short comment in generate-component-docs.rb

* fix spelling
AndrooTheChen pushed a commit to discord/vector that referenced this pull request Sep 23, 2024
…0498)

* set type_override for ascii_char

* specially handle "docs::type_override" with "ascii_char" in generate-component-docs.rb

* add missing filds in csv.rs and coresponsing cue docs as well as short comment in generate-component-docs.rb

* fix spelling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: ci Anything related to Vector's CI environment domain: external docs Anything related to Vector's external, public documentation no-changelog Changes in this PR do not need user-facing explanations in the release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants