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.

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

Add a managed, safer API #16

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Add a managed, safer API #16

wants to merge 16 commits into from

Conversation

atifaziz
Copy link
Collaborator

This PR proposes to add a managed and safer API to the native imports from libstapsdt instead of exposing them directly. The libstapsdt API doesn't provide much guards against misuse and since this could crash the host process, it warrants something that's fairly lightweight and safe.

Apart from adding wrapper types and guards, it also adds a strong-type approach to registering probes and firing with arguments.

First and foremost, all types under the Libstapsdt namespace are now private.

The central class is Provider with methods to initialise, load, unload and add probes. Destroying a provider happens by disposing an instance of Provider. Provider therefore implements IDisposable by virtue of being a SafeHandle, which is the most robust way to ensure resources are freed (even eventually during finalization if not done explicitly). This is the only (small) class that requires a heap allocation.

Provider has methods for defining probes:

partial class Provider
{
    public Probe<T> AddProbe<T>(string name) { /* ... */ }
    public Probe<T1, T2> AddProbe<T1, T2>(string name) { /* ... */ }
    public Probe<T1, T2, T3> AddProbe<T1, T2, T3>(string name) { /* ... */ }
    public Probe<T1, T2, T3, T4> AddProbe<T1, T2, T3, T4>(string name) { /* ... */ }
    public Probe<T1, T2, T3, T4, T5> AddProbe<T1, T2, T3, T4, T5>(string name) { /* ... */ }
    public Probe<T1, T2, T3, T4, T5, T6> AddProbe<T1, T2, T3, T4, T5, T6>(string name) { /* ... */ }
}

There are 6 corresponding lightweight structures representing probes:

public readonly partial record struct Probe;
public readonly partial record struct Probe<T>;
public readonly partial record struct Probe<T1, T2>;
public readonly partial record struct Probe<T1, T2, T3>;
public readonly partial record struct Probe<T1, T2, T3, T4>;
public readonly partial record struct Probe<T1, T2, T3, T4, T5>;
public readonly partial record struct Probe<T1, T2, T3, T4, T5, T6>;

The generic nature of these ensure strong-typed arguments. The type parameters are required to implement IArgType and IFireArgLong, which are zero-cost interfaces for mapping and conversion purposes. Implementations of these are provided on types, one for each member of ArgType_t, like UInt8Arg, Int8Arg, etc.

Since the various probe types are struct types, the bulk of their implementation is generated code using T4 templates (a source generator would be another option to consider in the future).

The gist of using the API is still pretty simple:

using System.Threading;
using DynamicProbes;

using var provider = Provider.Init("foo");
var probe = provider.AddProbe<Int64Arg>("bar");
_ = provider.Load();
while (true)
{
    probe.Active?.Fire(DateTime.Now.Ticks);
    Thread.Sleep(TimeSpan.FromSeconds(0.75));
}

The probe has an Active property that returns null if the probe isn't enabled and therefore can be combined with the null-conditional operator (?.) to succinctly avoid any cost of firing.

The API is also unit-tested. Testing native imports is near impossible or would require heavy work (like writing a mock libstapsdt.so in C), so the strategy chosen instead is to include DynamicProbes sources into the test project except Libstapsdt.cs. Then have an alternative implementation in the test project that allows mocking (unfortunately, this required some new mocking infrastructure for static code).


Things to do before publishing this PR as a draft:

  • Complete unit tests
  • Polish tests

@gukoff
Copy link
Owner

gukoff commented Sep 27, 2024

The probe has an Active property that returns null if the probe isn't enabled and therefore can be combined with the null-conditional operator (?.) to succinctly avoid any cost of firing.

Smart! I couldn't think of a way to do this efficiently without introducing new methods.

Conflicts resolved:

- DynamicProbes.sln
- eg/Program.cs
- src/DynamicProbes.csproj
- src/Libstapsdt.cs
@atifaziz atifaziz changed the title 🚧 Add a managed, safer API Add a managed, safer API Oct 3, 2024
@atifaziz atifaziz marked this pull request as ready for review October 3, 2024 16:52
@atifaziz
Copy link
Collaborator Author

atifaziz commented Oct 3, 2024

@gukoff What do you think of this API shape?

The PR was only in draft because I had the following to-do items:

  1. Complete unit tests
  2. Polish tests

However, the API shape won't change so I think it's good to consider this PR published, have it reviewed and then address the to-do items as a follow-up; they only contribute to increasing coverage (1) and refactoring (2), but it's important to agree on the overall direction.

Assuming you're okay with the direction, you can request that the to-do items be addressed and then we can bring everything in with one merge.

@atifaziz
Copy link
Collaborator Author

atifaziz commented Oct 3, 2024

@gukoff The benchmarks on this branch are updated but seem to be failing due to the (403) error, “Resource not accessible by integration”; the resource in question being gukoff/github-action-benchmark@short-floats.

@gukoff
Copy link
Owner

gukoff commented Oct 10, 2024

Hey, I'll review next week when I'm back from vacation. Sorry it's taking so long!

@gukoff gukoff self-requested a review October 19, 2024 12:36
? $"<{string.Join(", ", typeParams)}>"
: string.Empty;

string Constraints(string none, Func<string, string> singleton, Func<IEnumerable<string>, string> many)
Copy link
Owner

Choose a reason for hiding this comment

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

With the current usage, this method looks equivalent to:

return many(typeParams).Replace("\n", Environment.NewLine);

@@ -0,0 +1,112 @@
<#@ template debug="false" hostspecific="true" language="C#" #>
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
<#@ template debug="false" hostspecific="true" language="C#" #>
<#@ template debug="false" hostspecific="false" language="C#" #>

If I understand correctly, this is unnecessary

Comment on lines +30 to +47
<ItemGroup>
<None Update="Probes.g.tt">
<Generator>TextTemplatingFileGenerator</Generator>
<LastGenOutput>Probes.g.cs</LastGenOutput>
</None>
</ItemGroup>

<ItemGroup>
<Service Include="{508349b6-6b84-4df5-91f0-309beebad82d}" />
</ItemGroup>

<ItemGroup>
<Compile Update="Probes.g.cs">
<DesignTime>True</DesignTime>
<AutoGen>True</AutoGen>
<DependentUpon>Probes.g.tt</DependentUpon>
</Compile>
</ItemGroup>
Copy link
Owner

Choose a reason for hiding this comment

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

If I understand correctly, this is metainformation about the sources for the build task, but it won't regenerate if the template changes. At least on my Ubuntu, dotnet build doesn't trigger Probes.g.cs regeneration.

One could define a pre-build task to do exactly that. But it will be noisy if we keep the generated at {datetime}... line in the generated file.

CI can still check if the generated source is in sync with the template by making an exception for generated at {datetime} in the diff.

What do you think? Is there a best practice for keeping T4 templates in sync with the generated files?


public interface IFireArgLong
{
long UncheckedValue { get; }
Copy link
Owner

Choose a reason for hiding this comment

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

We always call the function from the C library with the long arguments, even if the probe was registered as Probe<Int8, Int8>.

I wonder if it's safe to pass the function parameters to C as 4-byte types; and if yes, why allow registering the probe as unit8 in the first place?

@gukoff
Copy link
Owner

gukoff commented Oct 20, 2024

Cool stuff, partial classes that are half-generated is something new for me!

The generic nature of these ensure strong-typed arguments. The type parameters are required to implement IArgType and IFireArgLong, which are zero-cost interfaces for mapping and conversion purposes

Interesting - why are they zero-cost?

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