Conversation
back to original clps version.
…r than hijacking ClpStringT.
+ many code and doc-string tweaks.
objects. Log-surgeon search side updated, but clp side changes not complete (may be in broken state).
different types); small refactoring.
Keep throws at the root usage to ensure nothing is missed. This reverts commit b46a68a.
bugs, but still need negation and leaf position support.
…on; Updates log-surgeon build chain.
gibber9809
left a comment
There was a problem hiding this comment.
Partial review focusing mostly on the parsing/write side. Should be mostly small changes, though I did spot one potential bug with find_log_shape_id/ClpMatcher that may have slightly bigger scope.
Also took a look at log-converter since I ran into a crash trying to use this branch's version.
| * - Forwards `m_archive_writer->update_log_shape_dict`'s return values on failure. | ||
| * - Forwards `m_archive_writer->update_parent_rule_shapes`'s return values on failure. | ||
| */ | ||
| auto parse_str_field( |
There was a problem hiding this comment.
Minor nit - I think the name of this function might be a little bit too generic, maybe something like parse_log_surgeon_string_field or parse_log_message might be a bit better? (Or at least parse_string_field if you disagree with those).
There was a problem hiding this comment.
That is fair, I'm also not happy with that name. Iirc, I used to have something closer to parse_log_message, but I was thinking that really it can parse any string not just log messages. parse_unstructured_string_field (or unstructured_text_field) is probably might be more accurate, but I'm fine with parse_log_message since realistically that is the purpose.
If you have a preference to anything lmk and I'll switch to that.
| /** | ||
| * An open `ParentRule` unordered object scope during `parse_str_field`. `match` identifies the | ||
| * scope within a `parse_str_field` call. `schema_start` is the scopes starting position in the | ||
| * schema and used when closing the unordered object. | ||
| */ | ||
| struct ParentScope { | ||
| log_surgeon::Match const* match; | ||
| size_t schema_start; | ||
| SchemaNode::id_t tree_node_id; | ||
| }; |
There was a problem hiding this comment.
I think that the schema_start name is mechanically accurate, but maybe not the clearest terminology since it mixes mechanism with functionality.
Instead of schema_start, maybe something like scope_handle, schema_handle, or rule_handle would make the functionality more clear? schema_start is aligned more with how we've been naming this in the rest of the code, but I don't think our existing naming is that good for this feature.
There was a problem hiding this comment.
Ya I don't like the name either (arguably it isn't super accurate either because that index isn't the "start of a schema" it is the scope's start within the schema).
I feel like I always associate "handle" with something that wraps a resource/object. I don't think I've ever used it for an index. What do you think of scope_schema_idx, scope_start_schema_idx. I got pretty confused working with schema entries so I personally would prefer there be some sort of relationship. At one point I was trying to refactor this stuff even more, but it was taking too long.
If you feel strongly about any name, I'll go with what you prefer.
| auto convert_string_to_double(std::string_view raw, double& converted) -> bool { | ||
| auto const res{fast_float::from_chars(raw.begin(), raw.end(), converted)}; | ||
| return res.ptr == raw.end() && std::errc{} == res.ec; | ||
| } | ||
|
|
There was a problem hiding this comment.
Beyond the scope of this PR, but I'm making a note to check if this fast_float library can help speed up the restore_encoded_float utility from clp-s.
There was a problem hiding this comment.
I think that is worthwhile. Fwiw, the reason I left fast_float in is because I did notice a non-trivial improvement when the encoding was still buggy (and trying to convert everything).
| log_msg_node_id, | ||
| *m_archive_writer | ||
| )}; | ||
| SchemaNode::id_t node_id{0}; |
There was a problem hiding this comment.
| SchemaNode::id_t node_id{0}; | |
| SchemaNode::id_t node_id{-1}; |
Should probably use -1 or std::optional<SchemaNode::id_t>, just because 0 is a valid node ID, even if by construction the leaf node here should never be node 0.
| [[nodiscard]] auto get_next_id() const -> uint64_t { return m_next_id; } | ||
|
|
There was a problem hiding this comment.
| [[nodiscard]] auto get_next_id() const -> uint64_t { return m_next_id; } |
Not used, as far as I can tell?
| // The closing placeholder delimiter is always the next '%' after the opening one as a | ||
| // column name cannot contain a delimiter. |
There was a problem hiding this comment.
Is this (making sure rule names can't contain %) enforced when parsing the spec file in log-surgeon right now?
| if (std::string_view::npos == close) { | ||
| result.emplace_back(Segment::Type::Literal, shape.substr(open)); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Can we hit this case for a valid text shape?
| compressor.write_numeric_value(m_parent_rule_shapes.size()); | ||
| for (auto const& shape : m_parent_rule_shapes) { | ||
| compressor.write_numeric_value(shape.m_name.size()); | ||
| compressor.write_string(shape.m_name); | ||
| compressor.write_numeric_value(shape.m_start); | ||
| compressor.write_numeric_value(shape.m_size); |
There was a problem hiding this comment.
| compressor.write_numeric_value(m_parent_rule_shapes.size()); | |
| for (auto const& shape : m_parent_rule_shapes) { | |
| compressor.write_numeric_value(shape.m_name.size()); | |
| compressor.write_string(shape.m_name); | |
| compressor.write_numeric_value(shape.m_start); | |
| compressor.write_numeric_value(shape.m_size); | |
| compressor.write_numeric_value<uint64_t>(m_parent_rule_shapes.size()); | |
| for (auto const& shape : m_parent_rule_shapes) { | |
| compressor.write_numeric_value<uint64_t>(shape.m_name.size()); | |
| compressor.write_string(shape.m_name); | |
| compressor.write_numeric_value<uint64_t>(shape.m_start); | |
| compressor.write_numeric_value<uint64_t>(shape.m_size); |
Just because if we compile this for a 32-bit target (like wasm) we can accidentally change the archive format if we're writing/reading size_t directly. Could also choose uint32_t or something here, just need to be explicit with whatever we choose here + on the read side.
| namespace clpp { | ||
| auto LogShapeStat::compress(clp_s::ZstdCompressor& compressor) const | ||
| -> ystdlib::error_handling::Result<void> { | ||
| compressor.write_numeric_value(m_count); |
There was a problem hiding this comment.
| compressor.write_numeric_value(m_count); | |
| compressor.write_numeric_value<uint64_t>(m_count); |
Same as my other comment -- should update the read and write side to use some explicit integer type besides size_t.
| template <typename Element, typename Index> | ||
| auto Array<Element, Index>::compress(clp_s::ZstdCompressor& compressor) | ||
| -> ystdlib::error_handling::Result<void> { | ||
| compressor.write_numeric_value(m_array.size()); |
There was a problem hiding this comment.
| compressor.write_numeric_value(m_array.size()); | |
| compressor.write_numeric_value<uint64_t>(m_array.size()); |
Same as other comments -- need to use explicit integer type for read/write side to avoid issue with size_t changing size between 32/64-bit platforms.
Description
Adds CLP+ (
clpp), an experimental extension ofclp-sthat parses unstructured text logs into structured, query-able fields using log-surgeon. Enabled via--experimental; existingclp-sbehaviour is unchanged when the flag is absent.CLP+ storage & compression
%log-surgeon.qualified.leaf_rule_name%placeholders. (Similar to CLP log types.)--experimental. Reading a CLP+ archive without it is an error.int/floatleaves are encoded as native numeric columns, enabling range queries.Search
shape(txt)to match against the shape oftxtwhere it is a log message or parent rule field (e.g.shape(msg): "INFO * static log text * %qualified.leaf_rule_name% *").message.blockID.genStamp), including wildcards and numeric comparisons.-DCLP_BUILD_CLPP_DECOMPOSITION=ON(defaultOFF).stats.archives,stats.log_shapes, andstats.schema_treequeries for inspecting archive metadata.Projection
Projectionto track a per-node mode mask (Value/Shape/Decompose) rather than a flat column set, so a single query can request raw text, the shape, and decomposed leaves at different nesting levels.shape(txt): outputtxt's shape as{"txt": {"shape": "..."}}decompose(txt): outputtxt's shape and all ofx's leaf rule fields as{"txt": {"shape": "...", "leaf": [...]}}.Documentation
Build & tests
fast_floatdependency.integration-tests/tests/binary_tests/test_clpp.pycovering round-trip, search, projection, and value encoding; decomposition-dependent tests are skipped unless the build flag is set.clp-sunit tests to run against both plain and CLP+ archives.Checklist
breaking change.
Validation performed
shape(), and non-wildcard message and parent rule queries.