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

Clog improvements #766

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Clog improvements #766

wants to merge 2 commits into from

Conversation

enescakir
Copy link
Member

@enescakir enescakir commented Oct 22, 2023

Add a special clog representation for Sequel::Model

We redact encrypted and lengthy columns from the .inspect output in our models. It would be wisely to apply the same redaction to our clog metadata as well.

Also, if the Sequel::Model is provided as metadata, the model automatically determines the key name from its table name and returns it with redacted values. { strand: strand.values } can be abbreviated as { strand }.

Allow to log additional fields with models in clog

Sometimes, the model data is insufficient, and we may need to add extra fields to the clog's metadata. The new array metadata feature achieves this.

When the metadata is an array, it parses each element sequentially. If the element is a Sequel::Model, it obtains its clog representation, similar to the previous commit. If the element is a hash, it merges with the intermediate hash.

For example, [strand, {field1: "a", field2: "b"}] expands to {strand: {id: 123, label: "start"}, field1: "a", field2: "b"}.

@enescakir enescakir requested a review from fdr October 22, 2023 13:08
@enescakir enescakir self-assigned this Oct 22, 2023
@fdr
Copy link
Collaborator

fdr commented Oct 23, 2023

I think we can include-by-default.

NetAddr can be handled specially as to_s, until we fork that thing and start changing some stuff (the #inspect rendering is probably the first priority, there).

Now, one idea I had different than you, I'm not sure if it's better or worse, and it's not mutually exclusive, was to support an array return from the block, so you could do this:

Clog.emit('msg') do
  [my_model, { my_custom_field: ... }]
end

Also, supporting an abbreviated form when there are no custom fields:

Clog.emit('msg') {  my_model }

By handling Sequel::Model in the type switch, the model would automatically determinate the key name it should have in the emitted JSON, which until now we have been typing by hand, e.g. strand: strand.values). You can run a snakification algorithm or use the table name of the model.

For Sequel::Model, using the encryption metadata can prune some more dangerous than necessary output. Also, long fields can be omitted as well.

There's still some use for in-hash special model handling, imagine, for example:

Clog.emit("failing over") do
  { from_server: from_server, to_server: to_server } 
end

But, it's pretty rare.

In the future, we may want to add a protocol to suppress the emission of dangerous fields that, for whatever reason, are not encrypted (like PII), or of those that are too expensive to emit. Or even has special renderings (a denser digest of a public key, for example) But I think there's a lot of time for that.

@fdr
Copy link
Collaborator

fdr commented Oct 23, 2023

Unrelatedly, I also go back and forth whether the block form I used for the values in Clog.emit was a good idea, so, if you are finding it causing friction or have another good idea how to use the one and only block input we get, let's consider elimination, instead doing the more traditional: Clog.emit("foo", meta: "data", model) style.

I also have some terrible ideas about how to make Clog.debug useful. Well, the idea is to be able to toggle individual strands into debug mode, by putting a boolean on strand, rather than by changing the whole application to debug (which is never going to happen in production, so then the use is narrowed to development, which indeed quite narrow). Maybe it's terrible, I dunno.

Then the whole idea of not calling a block passed to emit becomes useful.

@enescakir enescakir force-pushed the clog-metadata branch 6 times, most recently from 2ff7966 to 52f6429 Compare November 25, 2023 20:43
@enescakir enescakir changed the title Add a special clog representation for Sequel::Model Clog improvements Nov 25, 2023
@enescakir
Copy link
Member Author

@fdr I have 6 separate commits based on your feedbacks. Can you take a look again?

lib/clog.rb Outdated Show resolved Hide resolved
model/github_runner.rb Outdated Show resolved Hide resolved
@enescakir enescakir force-pushed the clog-metadata branch 3 times, most recently from 0ec834d to 237da61 Compare December 27, 2023 22:01
@enescakir enescakir marked this pull request as ready for review December 27, 2023 22:01
@enescakir enescakir requested a review from fdr December 27, 2023 22:01
@enescakir
Copy link
Member Author

@fdr What do you think about the latest version?

Copy link
Collaborator

@fdr fdr left a comment

Choose a reason for hiding this comment

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

Let's get it in, at long last.

We redact encrypted and lengthy columns from the .inspect output in our
models. It would be wisely to apply the same redaction to our clog
metadata as well.

Also, if the `Sequel::Model` is provided as metadata, the model
automatically determines the key name from its table name and returns it
with redacted values. `{ strand: strand.values }` can be abbreviated as
`{ strand }`.
Sometimes, the model data is insufficient, and we may need to add extra
fields to the clog's metadata. The new array metadata feature achieves
this.

When the metadata is an array, it parses each element sequentially. If
the element is a Sequel::Model, it obtains its clog representation,
similar to the previous commit. If the element is a hash, it merges with
the intermediate hash.

For example, `[strand, {field1: "a", field2: "b"}]` expands to `{strand:
{id: 123, label: "start"}, field1: "a", field2: "b"}`.
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

2 participants