feat(log_to_metric transform): support tags expansion similar to `labels expansion` for loki sink #21939

titaneric · 2024-12-03T17:12:20Z

Summary

This PR aims to support tags expansion for log_to_metric transform similar Loki's sink's labels expansion. As a new vector contributor, I copy the code from Loki sink and add unit test cases for these changes. If the further code refactor or docs are needed, I would immediately apply the changes. I would like to hear there's a more better implementation to this feature, and I am welcome to hear any comments.

Change Type

Bug fix
New feature
Non-functional (chore, refactoring, docs)
Performance

Is this a breaking change?

Yes
No

How did you test this PR?

make test

Does this PR include user facing changes?

Yes. Please add a changelog fragment based on our guidelines.
No. A maintainer will apply the "no-changelog" label to this PR.

Checklist

Please read our Vector contributor resources.
If this PR introduces changes Vector dependencies (modifies Cargo.lock), please
run dd-rust-license-tool write to regenerate the license inventory and commit the changes (if any). More details here.

References

Close #14744.

Partially solve #15332 ?

bits-bot · 2024-12-03T17:12:25Z

All committers have signed the CLA.

pront · 2024-12-03T19:23:59Z

Hi @titaneric, thank you for this contribution. I see this is marked as draft, feel free to give me a shout when you want a review.

titaneric · 2024-12-06T02:35:49Z

There is a testcase named variants::disk_v2::tests::invariants::reader_deletes_data_file_around_record_id_wraparound did not pass, and it seems that it's not related to my change.

I have added new testcase borrowed from loki's sink. @pront Please take your time to review it.

pront

Thanks @titaneric, this change makes sense to me 👍

Left a few comments. Also, please run make generate-component-docs

src/transforms/log_to_metric.rs

changelog.d/21939_log_to_metric_tags_expansion.feature.md

src/transforms/log_to_metric.rs

titaneric · 2024-12-23T04:26:15Z

I have moved the core expansion function to common mod, and adopt them in both loki sink and log_to_metrics transforms. Please review them if you are available!

drichards-87

Left a very minor suggestion from Docs and approved the PR.

website/cue/reference/components/transforms/base/log_to_metric.cue

titaneric · 2025-01-03T12:13:56Z

@pront Would you please take a look at this PR when you are available?

pront · 2025-01-03T17:06:29Z

@pront Would you please take a look at this PR when you are available?

Yes, it's on my radar.

pront

Thank you @titaneric, this is looking good. Left some suggestions for minor improvements.

src/common/expansion.rs

src/sinks/loki/sink.rs

src/transforms/log_to_metric.rs

pront · 2025-01-03T23:48:47Z

src/sinks/loki/sink.rs

-            } else {
-                static_labels.insert(key_s, value_s);
-            }
+            let _ = pair_expansion(key_s, value_s, &mut static_labels, &mut dynamic_labels);


Hm, should we check if this returned an error and if so, continue;? Let's be extra careful to not change the loki sink behavior.

I guess we could leave it unchanged? It's basically do the same thing as expanding structured_metadata.

I am referring to this existing code: https://github.com/vectordotdev/vector/blob/master/src/sinks/loki/sink.rs#L195

It's hard to follow without checking it out locally (too many nested blocks in this code).

Edit:
Can you just double check that the behavior is equivalent?
As long as we figure this out this PR is good to go.

ok, let me double-check it

I think they are equivalent. The original loki sink continues the next iteration while having the serde error in the for loop. On the other hand, my implementation return the error from the pair_expansion function, and the caller side just drop the error in the for loop. As a result, they do not continue to make further progress while having the serde error.

Honestly, I think loki sink needs some comments to explain, or handle the error properly instead of dropping it.

Honestly, I think loki sink needs some comments to explain, or handle the error properly instead of dropping it

Agreed!

src/common/expansion.rs

pront · 2025-01-06T15:58:31Z

src/common/expansion.rs

    static_pairs: &mut HashMap<String, String>,
    dynamic_pairs: &mut HashMap<String, String>,
 ) -> Result<HashMap<String, String>, serde_json::Error> {
    let mut expanded_pairs = HashMap::new();
    if let Some(opening_prefix) = key_s.strip_suffix('*') {
        let output: Result<serde_json::map::Map<String, serde_json::Value>, serde_json::Error> =
-            serde_json::from_str(value_s.clone().as_str());
+            serde_json::from_str(value_s);


✨ nice we got rid of a clone call

pront · 2025-01-06T16:00:56Z

src/sinks/loki/sink.rs

-            } else {
-                static_labels.insert(key_s, value_s);
-            }
+            let _ = pair_expansion(key_s, value_s, &mut static_labels, &mut dynamic_labels);


I am referring to this existing code: https://github.com/vectordotdev/vector/blob/master/src/sinks/loki/sink.rs#L195

It's hard to follow without checking it out locally (too many nested blocks in this code).

Edit:
Can you just double check that the behavior is equivalent?
As long as we figure this out this PR is good to go.

titaneric · 2025-01-06T16:29:28Z

It appears that test suite does not pass, let me fix it and double-check the equivalence of changed code.

titaneric · 2025-01-07T04:36:38Z

I think log_to_metric transform deserve another PR to update the documents.

titaneric · 2025-01-07T15:27:27Z

I have updated the example to adopt tags expansion feature.

rendered page:

Not sure whether it's a known issue or not, I notice that metric option could not be properly rendered on the documents, and it appears that it also affect the static_metrics docs. It seems that the page builder could not support array of object in cue, and I would open another issue to report it.

preview page:

cue:

titaneric · 2025-01-07T16:49:20Z

Open #22136 to discuss the document render bugs.

pront · 2025-01-07T17:38:54Z

It appears that test suite does not pass, let me fix it and double-check the equivalence of changed code.

I think your latest changes fixed this.

titaneric · 2025-01-11T02:36:41Z

Hi @pront , is it possible this PR would be merged in the upcoming days?

titaneric · 2025-01-13T17:27:01Z

@pront , thanks for merging this PR, and I just find another related issue at #7565 which I believed it could be closed!

…els expansion` for loki sink (vectordotdev#21939) * Try tag expansion * Add tags expansion test * Extract dynamic labels into tags result * Update tags expansion testcase * Refactor render_tag_into * Add multi_value_tags_expansion_yaml test * Add basic docs for tags * Add change log * Rename changelog file * rename labels to tags and inset the tag value in render_tag_into * Remove debug msg * Add colliding tags test * Add source reference * fix comment and changelog * Extract pair expansion to common mod * Adopt pair expansion in loki sink * Generate component docs * Add PairExpansion Error * Extend the result with extended pairs * Update docs * Re-generate docs * Add docs for pair_expansion and change signature to string literal * Update signature for render_tag_into * Handle the `pair_expansion` error in caller side * Better error handling in `pair_expansion` * Update example

titaneric requested a review from a team as a code owner December 3, 2024 17:12

github-actions bot added the domain: transforms Anything related to Vector's transform components label Dec 3, 2024

titaneric changed the title ~~feat(log_to_metric transformer): support tags expansion similar to labels expansion for loki sink~~ feat(log_to_metric transform): support tags expansion similar to labels expansion for loki sink Dec 3, 2024

titaneric marked this pull request as draft December 3, 2024 17:32

titaneric force-pushed the feat/tags-expansion branch from 4135b02 to 24a90b5 Compare December 6, 2024 02:09

titaneric marked this pull request as ready for review December 6, 2024 02:33

pront reviewed Dec 11, 2024

View reviewed changes

src/transforms/log_to_metric.rs Outdated Show resolved Hide resolved

changelog.d/21939_log_to_metric_tags_expansion.feature.md Outdated Show resolved Hide resolved

src/transforms/log_to_metric.rs Outdated Show resolved Hide resolved

titaneric and others added 14 commits December 22, 2024 14:44

Try tag expansion

1fc4d4d

Add tags expansion test

f49b068

Extract dynamic labels into tags result

7a94bd1

Update tags expansion testcase

e85cb44

Refactor render_tag_into

7c39d98

Add multi_value_tags_expansion_yaml test

788a257

Add basic docs for tags

293b15a

Add change log

ba98fb2

Rename changelog file

c4bba1b

rename labels to tags and inset the tag value in render_tag_into

8ced9a8

Remove debug msg

3884f83

Add colliding tags test

21a532c

Add source reference

9bf3219

fix comment and changelog

5957703

titaneric force-pushed the feat/tags-expansion branch from 6009cfe to 5957703 Compare December 22, 2024 05:44

Extract pair expansion to common mod

c23fb31

github-actions bot added the domain: sinks Anything related to the Vector's sinks label Dec 22, 2024

titaneric force-pushed the feat/tags-expansion branch from e1135f9 to 27089c3 Compare December 22, 2024 18:19

Adopt pair expansion in loki sink

3a10f42

titaneric force-pushed the feat/tags-expansion branch from 27089c3 to 3a10f42 Compare December 22, 2024 18:21

Extend the result with extended pairs

6af30a4

drichards-87 approved these changes Dec 23, 2024

View reviewed changes

website/cue/reference/components/transforms/base/log_to_metric.cue Outdated Show resolved Hide resolved

titaneric and others added 2 commits December 24, 2024 01:46

Update docs

c337c04

Re-generate docs

fbdebbb

pront reviewed Jan 3, 2025

View reviewed changes

titaneric added 2 commits January 4, 2025 12:37

Add docs for pair_expansion and change signature to string literal

f08e7ff

Update signature for render_tag_into

97a8756

titaneric requested a review from pront January 4, 2025 05:09

pront reviewed Jan 6, 2025

View reviewed changes

pront approved these changes Jan 6, 2025

View reviewed changes

titaneric added 2 commits January 7, 2025 00:55

Handle the pair_expansion error in caller side

a4ecc09

Better error handling in pair_expansion

32ed91d

titaneric force-pushed the feat/tags-expansion branch from 5005b31 to 32ed91d Compare January 6, 2025 17:14

Update example

5d4b386

pront added this pull request to the merge queue Jan 13, 2025

Merged via the queue into vectordotdev:master with commit ff8dfb7 Jan 13, 2025
40 checks passed

titaneric deleted the feat/tags-expansion branch January 17, 2025 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(log_to_metric transform): support tags expansion similar to `labels expansion` for loki sink #21939

feat(log_to_metric transform): support tags expansion similar to `labels expansion` for loki sink #21939

titaneric commented Dec 3, 2024 •

edited

Loading

bits-bot commented Dec 3, 2024 •

edited

Loading

pront commented Dec 3, 2024

titaneric commented Dec 6, 2024

pront left a comment

titaneric commented Dec 23, 2024

drichards-87 left a comment

titaneric commented Jan 3, 2025

pront commented Jan 3, 2025

pront left a comment

pront Jan 3, 2025

titaneric Jan 4, 2025

pront Jan 6, 2025 •

edited

Loading

titaneric Jan 6, 2025

titaneric Jan 6, 2025 •

edited

Loading

pront Jan 13, 2025

pront Jan 6, 2025

pront Jan 6, 2025 •

edited

Loading

titaneric commented Jan 6, 2025

titaneric commented Jan 7, 2025

titaneric commented Jan 7, 2025 •

edited

Loading

titaneric commented Jan 7, 2025

pront commented Jan 7, 2025

titaneric commented Jan 11, 2025

titaneric commented Jan 13, 2025

feat(log_to_metric transform): support tags expansion similar to labels expansion for loki sink #21939

feat(log_to_metric transform): support tags expansion similar to labels expansion for loki sink #21939

Conversation

titaneric commented Dec 3, 2024 • edited Loading

Summary

Change Type

Is this a breaking change?

How did you test this PR?

Does this PR include user facing changes?

Checklist

References

bits-bot commented Dec 3, 2024 • edited Loading

pront commented Dec 3, 2024

titaneric commented Dec 6, 2024

pront left a comment

Choose a reason for hiding this comment

titaneric commented Dec 23, 2024

drichards-87 left a comment

Choose a reason for hiding this comment

titaneric commented Jan 3, 2025

pront commented Jan 3, 2025

pront left a comment

Choose a reason for hiding this comment

pront Jan 3, 2025

Choose a reason for hiding this comment

titaneric Jan 4, 2025

Choose a reason for hiding this comment

pront Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

titaneric Jan 6, 2025

Choose a reason for hiding this comment

titaneric Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

pront Jan 13, 2025

Choose a reason for hiding this comment

pront Jan 6, 2025

Choose a reason for hiding this comment

pront Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

titaneric commented Jan 6, 2025

titaneric commented Jan 7, 2025

titaneric commented Jan 7, 2025 • edited Loading

titaneric commented Jan 7, 2025

pront commented Jan 7, 2025

titaneric commented Jan 11, 2025

titaneric commented Jan 13, 2025

feat(log_to_metric transform): support tags expansion similar to `labels expansion` for loki sink #21939

feat(log_to_metric transform): support tags expansion similar to `labels expansion` for loki sink #21939

titaneric commented Dec 3, 2024 •

edited

Loading

bits-bot commented Dec 3, 2024 •

edited

Loading

pront Jan 6, 2025 •

edited

Loading

titaneric Jan 6, 2025 •

edited

Loading

pront Jan 6, 2025 •

edited

Loading

titaneric commented Jan 7, 2025 •

edited

Loading