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.

Sign up for GitHub

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

Jump to bottom

Fixup build-telemetry-ffi and cbindgen.toml #105

Merged
merged 2 commits into from
Feb 9, 2023
Merged

Fixup build-telemetry-ffi and cbindgen.toml #105

merged 2 commits into from
Feb 9, 2023

Conversation

pawelchcki
Copy link
Contributor

What does this PR do?

A brief description of the change being made with this pull request.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

How to test the change?

Describe here in detail how the change can be validated.

@pawelchcki pawelchcki requested a review from a team as a code owner February 2, 2023 13:29
ivoanjo
ivoanjo approved these changes Feb 3, 2023
View reviewed changes
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

build-telemetry-ffi.sh
@@ -0,0 +1,141 @@
#!/usr/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

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

I was diffing this and build-profiling-ffi.sh and noticed this is missing the changes from #101 . Not a blocker for this PR, but you may want to import them as well :)

ddtelemetry-ffi/cbindgen.toml
Comment on lines +23 to +26
[export.rename]
"ParseTagsResult" = "ddog_Vec_Tag_ParseResult"
"PushTagResult" = "ddog_Vec_Tag_PushResult"
"FILE" = "FILE"
Copy link
Member

Choose a reason for hiding this comment

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

There's a few more renames missing (there's quite a few telemetry-specific types that are being placed in the common namespace), but again this was already the case so it's more of an FYI ;)

@pawelchcki pawelchcki merged commit 510472d into main Feb 9, 2023
@pawelchcki pawelchcki deleted the pawel/fix_telemetry_build branch February 9, 2023 14:18
thedavl added a commit that referenced this pull request Feb 14, 2023
@thedavl
Squashed commit of the following:
6c24f99 
commit 72af2a0
Author: David Lee <thedavl2001@gmail.com>
Date:   Mon Feb 13 15:44:57 2023 -0800

    Trace Normalizer: add trace & trace chunk normalization (#109)

commit 510472d
Author: Pawel Chojnacki <pawelchcki@gmail.com>
Date:   Thu Feb 9 15:05:33 2023 +0100

    Fixup build-telemetry-ffi and cbindgen.toml (#105)

commit 711bd43
Author: David Lee <thedavl2001@gmail.com>
Date:   Wed Feb 8 15:18:24 2023 -0800

    Trace Normalizer: add service + env tag normalization (#106)

    * add service tag + env tag normalization + unit tests

commit ee4bba8
Author: Ivo Anjo <ivo.anjo@datadoghq.com>
Date:   Tue Feb 7 15:38:13 2023 +0000

    Remove unneeded "gem signout" step from Ruby release (#108)

    **What does this PR do?**:

    This PR removes the now-unneeded "gem signout" steps during the Ruby
    release process.

    **Motivation**:

    In #85, we changed the way that we authenticate with rubygems.org when
    pushing a new libdatadog release.

    I just did a release with this new code, and noticed that because
    we no longer log in, but just use a limited API key, the "gem signout"
    does not do anything and emits an error.

    Here's what I saw when I ran `docker-compose run push_to_rubygems`:

    ```
    ... preparation of packages goes here...

    ERROR:  You are not currently signed in.
    Please input 'libdatadog ruby release key' from 'Profiling - Falcon' Datadog 1Password:
    (...key...)
    Pushing gem to https://rubygems.org...
    You have enabled multi-factor authentication. Please enter OTP code.
    Code:   (...)
    Successfully registered gem: libdatadog (2.0.0.1.0)
    Pushing gem to https://rubygems.org...
    You have enabled multi-factor authentication. Please enter OTP code.
    Code:   (...)
    Successfully registered gem: libdatadog (2.0.0.1.0-x86_64-linux)
    Pushing gem to https://rubygems.org...
    You have enabled multi-factor authentication. Please enter OTP code.
    Code:   (...)
    Successfully registered gem: libdatadog (2.0.0.1.0-aarch64-linux)
    ERROR:  You are not currently signed in.
    ```

    Those two "ERROR:  You are not currently signed in" come from the
    "gem signout" steps, and what's why I'm removing them.

    **Additional Notes**:

    (N/A)

    **How to test the change?**:

    You can run `docker-compose run push_to_rubygems` and validate
    the errors will not show up again. This is safe because Rubygems
    does not allow re-releasing the same packages.

commit 8be5465
Author: Ivo Anjo <ivo.anjo@datadoghq.com>
Date:   Fri Feb 3 14:35:58 2023 +0000

    Package libdatadog v2.0.0 for Ruby (#107)

    **What does this PR do?**:

    This PR includes the changes documented in the
    "Releasing a new version to rubygems.org" part of the README:
    <https://github.com/DataDog/libdatadog/tree/main/ruby#releasing-a-new-version-to-rubygemsorg>

    **Motivation**:

    Enable Ruby to use libdatadog v2.0.0.

    **Additional Notes**:

    (N/A)

    **How to test the change?**:

    This was locally checked with the Ruby profiler branch that already
    supports libdatadog 2.

commit 73b8e2e
Author: David Lee <thedavl2001@gmail.com>
Date:   Thu Feb 2 14:01:07 2023 -0800

    Create a span normalizer skeleton, fully implement span name normalization (#100)

commit 097ea5d
Author: Levi Morrison <levi.morrison@datadoghq.com>
Date:   Thu Feb 2 10:32:39 2023 -0700

    refactor(profiling)!: less chance of request double free (#103)

commit e3887c3
Author: Pawel Chojnacki <pawelchcki@gmail.com>
Date:   Mon Jan 30 18:06:46 2023 +0100

    Fix CI warnings (#104)

    * Fix warnings

    * clippy fix

commit 19b7f69
Author: Levi Morrison <levi.morrison@datadoghq.com>
Date:   Fri Jan 27 10:34:37 2023 -0700

    refactor(profiling)!: create FFI Error type and remove `*Result_drop` methods (#95)

    * refactor(profiling)!: create FFI Error type

    This extracts an FFI Error type `ddog_Error`. It contains an FFI Vec
    internally:

    ```rust
    /// Please treat this as opaque; do not reach into it, and especially
    /// don't write into it!
    pub struct Error {
        /// This is a String stuffed into the vec.
        message: Vec<u8>,
    }
    ```

    FFI result types have been updated e.g.:

    ```rust
    pub enum SendResult {
        HttpResponse(HttpStatus),
        Err(Error),
    }
    ```

    Instead of reaching into the buffer, use these APIs:

    ```c
    /**
     * # Safety
     * Only pass null or a valid reference to an Error.
     */
    void ddog_Error_drop(struct ddog_Error *error);

    /**
     * Returns a CharSlice of the error's message that is valid until the error
     * is dropped.
     * # Safety
     * Only pass null or a valid reference to an Error.
     */
    ddog_CharSlice ddog_Error_message(const struct ddog_Error *error);
    ```

    * Drop *Result_drop functions

    The `*Result_drop` methods have been removed:

    - ddog_prof_Exporter_NewResult_drop
    - ddog_prof_Exporter_Request_BuildResult_drop
    - ddog_prof_Exporter_SendResult_drop
    - ddog_prof_Profile_AddResult_drop
    - ddog_prof_Profile_SerializeResult_drop
    - ddog_Vec_Tag_PushResult_drop

    And these were added instead:

    - ddog_Error_drop (+ ddog_Error_message to get the message)
    - ddog_prof_EncodedProfile_drop
    - ddog_prof_Exporter_Request_drop
    * Make a note about #[must_use]

    * Add ddog_prof_EncodedProfile_drop

    Co-authored-by: Ivo Anjo <ivo.anjo@datadoghq.com>

commit ed1ee92
Author: Florian Engelhardt <florian.engelhardt@datadoghq.com>
Date:   Fri Jan 27 17:19:30 2023 +0100

    fix link to contribution guide (#102)

commit 1c445fe
Author: Levi Morrison <levi.morrison@datadoghq.com>
Date:   Fri Jan 27 09:02:54 2023 -0700

    feat(build-profiling-ffi.sh): support CARGO_TARGET_DIR (#101)

commit 88899ba
Author: Levi Morrison <levi.morrison@datadoghq.com>
Date:   Fri Jan 27 08:43:11 2023 -0700

    fix: clippy lints from Rust v1.67.0 release (#99)

    * fix clippy::uninlined_format_args

    * fix clippy::seek_to_start_instead_of_rewind

commit f2d0ed0
Author: Levi Morrison <levi.morrison@datadoghq.com>
Date:   Wed Jan 25 15:32:33 2023 -0700

    feat(profiling)!: pass errors through more FFI functions (#90)

    This changes the return types for FFI functions:
     - `ddog_prof_Profile_add`
     - `ddog_prof_Exporter_Request_build`

    Adds new structs:
     - `ddog_prof_Profile_AddResult`
     - `ddog_prof_Exporter_Request_BuildResult`

    And adds functions to drop them:
     - `ddog_prof_Profile_AddResult_drop`
     - `ddog_prof_Exporter_Request_BuildResult_drop`

    Removes a now-unnecessary newtype definition of struct
    `Request(exporter::Request)` as cbindgen handles the refactored
    code.

commit c93b7c1
Author: Levi Morrison <levi.morrison@datadoghq.com>
Date:   Mon Jan 23 10:56:21 2023 -0700

    chore: update dependencies (#96)

    * chore: update dependencies

    This fixes a dependabot alert:
    https://github.com/DataDog/libdatadog/security/dependabot/6

    With the updated dependencies, there was a new deprecation:

    > warning: use of deprecated associated function
    > `chrono::TimeZone::timestamp`: use `timestamp_opt()` instead

    I replaced it with `timestamp_opt` and unwrapped it, which is exactly
    what the now-deprecated `timestamp` function does:
    https://github.com/chronotope/chrono/blob/378efb157d674c01761f538d4450705c2b1766a4/src/offset/mod.rs#L343

    They deprecated it because they are working to not panic internally.

    * Bump LICENSE-3rdparty.yml
Merge branch 'main' into origin/david.lee/agentless-use-trace-normalizer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivoanjo ivoanjo ivoanjo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pawelchcki @ivoanjo