Skip to content

de: tp: Add metrics v2 node#4756

Open
aMayzner wants to merge 2 commits intomainfrom
dev/mayzner/metrics-v2-node
Open

de: tp: Add metrics v2 node#4756
aMayzner wants to merge 2 commits intomainfrom
dev/mayzner/metrics-v2-node

Conversation

@aMayzner
Copy link
Member

TP: Adds ProtoToText experimental function that alows us to show textoprotos to users on export
DE: Adds metrics v2 node, with support for one metric column

@aMayzner aMayzner requested a review from a team as a code owner February 11, 2026 09:12
@github-actions
Copy link

github-actions bot commented Feb 11, 2026

// the new instance and ownership is transferred to the caller.
virtual base::Status CreateSummarizer(std::unique_ptr<Summarizer>* out) = 0;

// EXPERIMENTAL: Converts a proto from binary format to textproto format.
Copy link
Member

Choose a reason for hiding this comment

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

Uhhhhh why do you need this? This seems totally unnecessary.

// the new instance and ownership is transferred to the caller.
virtual base::Status CreateSummarizer(std::unique_ptr<Summarizer>* out) = 0;

// EXPERIMENTAL: Converts a proto from binary format to textproto format.
Copy link
Member

Choose a reason for hiding this comment

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

No sorry please don't add a function like this, this is totally not the right thing to do. You should be able to convert to text proto in JavaScript without going to trace processor. If you need to do it on c++ see how it's done for trace config and generalize. I've allowed ad-hoc changes on TP and not looked too closely but this is where I draw the line. I don't want to add random things in TP just because it's convenient for explore page.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add functionality to go from protojs to text proto without TP, but I don't get the strong reaction. So far, this is what we have been always doing. To get trace config, in recording page, in Data explorer. The idea of continuing with it was not so random.

Copy link
Member

Choose a reason for hiding this comment

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

No the reaction here is because you are basically just dumping a shortcut to do proto to prototext of any proto trace processor understands. That's not the job of trace processor and I think that should be reasonably clear

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants