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 better documentation for IResourceDetector and OltpExporter configuration #4230

Merged
merged 27 commits into from
Mar 31, 2023

Conversation

cromefire
Copy link
Contributor

@cromefire cromefire commented Feb 24, 2023

Fixes #4228.

Changes

  • Adds more documentation to make the configurability of the OLTP exporter more clear.
  • Better documentation for the IResourceDetector

For significant contributions please make sure you have completed the following items:

@cromefire cromefire requested a review from a team February 24, 2023 15:58
@cromefire cromefire changed the title Add optional IServiceProvider to ConfigureResource, WithTracing & WithMetrics Add add better documentation for IResourceDetector and OltpExporter configuration Feb 24, 2023
@reyang
Copy link
Member

reyang commented Feb 24, 2023

@cromefire would you update the PR title please? (this PR is exposing new API, which is more than "add better documentation")

@cromefire cromefire changed the title Add add better documentation for IResourceDetector and OltpExporter configuration Expose ISerivceProvider variant of AddDetector... Feb 24, 2023
@cromefire cromefire changed the title Expose ISerivceProvider variant of AddDetector... Expose ISerivceProvider variant of AddDetector Feb 24, 2023
@cromefire
Copy link
Contributor Author

Also added #4139 to the fixes list.

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Blocking temporarily to prevent accidental merges, as we are in the process of stable release. Will unblock once the release process is done.

@cijothomas cijothomas dismissed their stale review February 24, 2023 22:36

1.4 release work is done.

…lbacks

# Conflicts:
#	src/OpenTelemetry/.publicApi/net462/PublicAPI.Unshipped.txt
#	src/OpenTelemetry/.publicApi/net6.0/PublicAPI.Unshipped.txt
#	src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt
#	src/OpenTelemetry/.publicApi/netstandard2.1/PublicAPI.Unshipped.txt
@cromefire cromefire changed the title Expose ISerivceProvider variant of AddDetector Add better documentation for IResourceDetector and OltpExporter configuration Mar 6, 2023
@cromefire
Copy link
Contributor Author

Made this into the PR for the documentation

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Mar 15, 2023
@cromefire
Copy link
Contributor Author

Just waiting for merge, bot

@github-actions github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Mar 16, 2023
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Mar 24, 2023
@cromefire
Copy link
Contributor Author

Go away bot, still waiting for merge.

@github-actions github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Mar 25, 2023
Comment on lines 94 to 102
public class MyResourceDetector : IResourceDetector
{
public Resource Detect()
{
return ResourceBuilder.CreateEmpty()
.AddService("your service name")
.Build();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

What if we made this slightly more interesting by showing something being injected?

public class MyResourceDetector : IResourceDetector
{
    private readonly IWebHostEnvironment webHostEnvironment;

    public MyResourceDetector(IWebHostEnvironment webHostEnvironment)
    {
        this.webHostEnvironment = webHostEnvironment;
    }

    public Resource Detect()
    {
        return ResourceBuilder.CreateEmpty()
            .AddService(serviceName: this.webHostEnvironment.ApplicationName)
            .AddAttributes(new Dictionary<string, object> { ["host.environment"] = this.webHostEnvironment.EnvironmentName })
            .Build();
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The environment is injected, I can add something else too if you want.

Copy link
Member

Choose a reason for hiding this comment

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

@cromefire I don't see any changes on the diff did you forget to push maybe? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I'm just too stupid to use the GitHub app... Will fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 30, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

@CodeBlanch CodeBlanch merged commit c5ddf5a into open-telemetry:main Mar 31, 2023
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.

Pass the IServiceProvider in ConfigureResource, WithTracing & WithMetrics
4 participants