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

rec: Add bindings to set arbitrary key-value metadata in logged messages #10491

Merged
merged 10 commits into from Jul 5, 2021

Conversation

chbruyand
Copy link
Member

@chbruyand chbruyand commented Jun 10, 2021

Short description

This PR adds new protobuf meta field that can contain arbitrary key-value metadata.

This still needs:

  • regression tests
  • the corresponding non FFI interface ?

New protobuf meta field can also be read as the following protov3 map:

  message MetaValue {
    repeated string stringVal = 1;
    repeated int64 intVal = 2;
  }

  map<string, MetaValue> meta = 22;

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • checked that this code was merged to master

@chbruyand chbruyand changed the title rec: Add bindings to set arbitrary key-value metadata in logged messages Draft: rec: Add bindings to set arbitrary key-value metadata in logged messages Jun 10, 2021
@chbruyand chbruyand marked this pull request as draft June 10, 2021 15:46
@chbruyand chbruyand changed the title Draft: rec: Add bindings to set arbitrary key-value metadata in logged messages rec: Add bindings to set arbitrary key-value metadata in logged messages Jun 10, 2021
Copy link
Member

@omoerbeek omoerbeek left a comment

Choose a reason for hiding this comment

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

Rest of review will follow once this is non-Draft.

.not-formatted Outdated Show resolved Hide resolved
@chbruyand chbruyand marked this pull request as ready for review June 18, 2021 09:53
@omoerbeek omoerbeek self-assigned this Jun 29, 2021
pdns/lua-recursor4.cc Outdated Show resolved Hide resolved
Copy link
Member

@omoerbeek omoerbeek left a comment

Choose a reason for hiding this comment

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

Looks good to me. A few nits and docs are missing. Also, it would be good if somebody more knowledgeable about ffi interfaces takes a look.

pdns/lua-recursor4.cc Outdated Show resolved Hide resolved
pdns/pdns_recursor.cc Outdated Show resolved Hide resolved
Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

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

Looks good, I wonder if we should exclude lua-recursor4.cc from the formatting, at least partially.

pdns/lua-recursor4.cc Outdated Show resolved Hide resolved
pdns/pdns_recursor.cc Outdated Show resolved Hide resolved
@chbruyand
Copy link
Member Author

I turned off the clang formating for the whole RecursorLua4::postPrepareContext() function and kept it on for the rest of the file. This looks indeed better like that.


.. function:: pdns_ffi_param_add_meta_single_string_kv(pdns_ffi_param_t* ref, const char* key, const char* val) -> void

.. versionadded:: 4.5.3
Copy link
Member

Choose a reason for hiding this comment

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

Does this need a backport? If so, the earliest version it's going to be in is 4.5.5, as 4.5.4 was just released without this. Otherweise the versionadded:: should be 4.6.0


.. function:: pdns_ffi_param_add_meta_single_int64_kv(pdns_ffi_param_t *ref, const char* key, int64_t val) -> void

.. versionadded:: 4.5.3
Copy link
Member

Choose a reason for hiding this comment

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

same here

Copy link
Member

@omoerbeek omoerbeek left a comment

Choose a reason for hiding this comment

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

LGTM!

@omoerbeek omoerbeek merged commit 2a4d645 into PowerDNS:master Jul 5, 2021
@omoerbeek omoerbeek added this to the rec-4.6.0 milestone Oct 13, 2021
@chbruyand chbruyand deleted the key-values-metadata branch January 19, 2024 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants