Skip to content

feat: add process storage#1

Draft
elsakeirouz wants to merge 9 commits intomasterfrom
elsa/add-process-storage
Draft

feat: add process storage#1
elsakeirouz wants to merge 9 commits intomasterfrom
elsa/add-process-storage

Conversation

@elsakeirouz
Copy link
Copy Markdown

No description provided.

@elsakeirouz elsakeirouz self-assigned this Apr 28, 2025
@elsakeirouz elsakeirouz force-pushed the elsa/add-process-storage branch from 6e3c376 to b673987 Compare April 28, 2025 13:29
Comment thread src/customlabels.c
#define cur_storage (custom_labels_current_set->storage)
#define cur_capacity (custom_labels_current_set->capacity)

__attribute__((retain))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why do we need retain ? I have not used this in C. Is it valid ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I mainly used it because they used it for the other exported variables. I am looking into it in more details now.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This seems to be used to prevent section
garbage collection by the linker, as explained here. It means that the variable's symbol will remain visible even if it is not referenced in the code. For process_storage, this is not very relevant since we do not care for it if it is not used. It makes more sense for the other exported variables.

Suggested change
__attribute__((retain))

Comment thread src/customlabels.h
* Set the process-level data.
*
* We want to keep this somewhat generic. For simplicity, for the time being,
* the tracer hands the library the bytes directly.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor: the sentence is strange. Proposal:
The tracer provides the bytes to the library. The library stores them until the proc_storage_free function is called.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Suggested change
* the tracer hands the library the bytes directly.
* the tracer provides the bytes to the library. The library stores them until the proc_storage_free function is called.

That sounds good.

Comment thread src/customlabels.c
#define cur_capacity (custom_labels_current_set->capacity)

__attribute__((retain))
unsigned char* process_storage = NULL;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should we store the size in association ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It is not needed currently because of the way we read the data. You can refer to this for an idea of the general format, and this for the format of the strings (service-name, runtime-id...)

@elsakeirouz elsakeirouz force-pushed the elsa/add-process-storage branch from b673987 to 84bc0b1 Compare April 30, 2025 12:24
* BUILD.bazel
* MODULE.bazel
@elsakeirouz elsakeirouz force-pushed the elsa/add-process-storage branch from ae397dc to 1bc7423 Compare April 30, 2025 13:11
@elsakeirouz elsakeirouz force-pushed the elsa/add-process-storage branch from aa2e823 to 5c5eaf2 Compare May 13, 2025 15:45
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.

4 participants