Skip to content
This repository has been archived by the owner on Jun 28, 2022. It is now read-only.

Enhance Tags API #49

Merged
merged 8 commits into from
Apr 13, 2022
Merged

Enhance Tags API #49

merged 8 commits into from
Apr 13, 2022

Conversation

morrisonlevi
Copy link
Collaborator

What does this PR do?

This adds back the tags FFI API and supports DD_TAGS parsing.

Fixes the exporter example.

Motivation

I want DD_TAGS parsing in PHP and it wasn't as simple as I hoped, so
trying to share the work here too.

Additional Notes

The test_dup test is the one that would previously sigsgev (sometimes).
Now that lifetimes and other things have been cleaned up, I can't get
it to sigsev anymore.

How to test the change?

Most ddprof_ffi_Vec_tag functions are the same as before. The new
function ddprof_ffi_Vec_tag_parse takes a CharSlice and returns a
ParseTagsResult:

#[repr(C)]
pub struct ParseTagsResult {
    tags: crate::Vec<Tag>,
    error_message: Option<Box<crate::Vec<u8>>>,
}

In this sense, it doesn't "fail" but returns a Vec<Tag> of successfully
parsed tags, and an error message summarizing everything that failed.

I've tested this locally with an updated branch of dd-prof-php.

Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

Left a bunch of tiny nitpicks, but overall it seems quite reasonable 😄

ddprof-exporter/src/tag.rs Outdated Show resolved Hide resolved
ddprof-exporter/src/tag.rs Outdated Show resolved Hide resolved
ddprof-exporter/src/tag.rs Show resolved Hide resolved
ddprof-exporter/src/tag.rs Outdated Show resolved Hide resolved
ddprof-ffi/src/tags.rs Show resolved Hide resolved
ddprof-ffi/src/tags.rs Outdated Show resolved Hide resolved
ddprof-ffi/src/tags.rs Show resolved Hide resolved
ddprof-ffi/src/tags.rs Outdated Show resolved Hide resolved
examples/ffi/exporter.cpp Outdated Show resolved Hide resolved
examples/ffi/exporter.cpp Outdated Show resolved Hide resolved
Base automatically changed from levi/cleanup to main April 12, 2022 16:29
morrisonlevi and others added 3 commits April 12, 2022 10:33
Co-authored-by: Ivo Anjo <ivo.anjo@datadoghq.com>
Ivo thinks this will give a better result for this edge, and it made
the code simpler, so I went for it.

Also run cargo fmt.
ddprof-exporter/src/tag.rs Outdated Show resolved Hide resolved
#[repr(C)]
pub struct ParseTagsResult {
tags: crate::Vec<Tag>,
error_message: Option<Box<crate::Vec<u8>>>,
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking. Would it make sense to add it as a comment, so that it shows up on ffi.h?

Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

ddprof-exporter/src/tag.rs Outdated Show resolved Hide resolved
Co-authored-by: Ivo Anjo <ivo.anjo@datadoghq.com>
@morrisonlevi morrisonlevi merged commit 280373e into main Apr 13, 2022
@morrisonlevi morrisonlevi deleted the levi/tags branch April 13, 2022 17:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants