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

Integrate CloudTrace with PubSub #9476

Closed
christopheblin opened this issue Dec 15, 2022 · 5 comments
Closed

Integrate CloudTrace with PubSub #9476

christopheblin opened this issue Dec 15, 2022 · 5 comments
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: question Request for information or clarification. Not an issue.

Comments

@christopheblin
Copy link

Is your feature request related to a problem? Please describe.

My problem is that when I publish a message from one cloud run and then consume the message from other cloud runs with a push subscription, then each consumer is a separate trace (defeating the purpose of distributed tracing because when I have an error in the pubsub consumer, it is not easy to go back to the previous trace)

See the next "More details" to see how I configure them

Describe the solution you'd like
I'd like a Google supported nuget package that takes care of distributed tracing in the PubSub scenario with a push endpoint in a cloud run

More details
I configure PubSub and CloudTrace in the first cloud run :

builder.Services.AddSingleton(sp => new PubSubPublisher(sp.GetRequiredService<ILogger<PubSubPublisher>>(), options, emulatorDetection));
builder.Services.AddGoogleDiagnosticsForAspNetCore()

Note : the PubSubPublisher class is just a wrapper above PublisherClient, see code below

I then have a push subscription in another cloud run (where I also have builder.Services.AddGoogleDiagnosticsForAspNetCore())

Note that I redacted some part of the code for "privacy"

[ApiController]
[Authorize(Policy = PubSubAuthentication.Scheme)]
[ApiExplorerSettings(IgnoreApi = true)]
public class StoreAuditController : ControllerBase
{
    private readonly ILogger<StoreAuditController> _logger;

    public StoreAuditController(ILogger<StoreAuditController> logger)
    {
        _logger = logger;
    }

    [HttpPost("/pubsub/xxx", Name = nameof(StoreAuditForXXX))]
    public void StoreAuditForXxx([FromBody] PubSubMessageEnveloppe o)
    {
       _logger.LogInformation("Received PubSub message {}", JsonSerializer.Serialize(o));
    }
}

Proposed "solution"

The nuget would

  1. automatically add an attribute X-Cloud-Trace-Context in the published message in function of the current trace context
  2. add an Http middleware that detects it is a PUSH endpoint (for ex, with [PubSubEndpoint] above the moethod) and automatically grabs the X-Cloud-Trace-Context inside the context

Do you think it can be done ? If so, could you point me to the doc where I can find answers to the TODO in the code below ?

Thanks for any pointers, I'm really frustrated 😄

Code

public class PubSubPublisher
{
    private readonly ILogger<PubSubPublisher> _logger;
    private readonly PublisherClient _publisherClient;

    public PubSubPublisher(ILogger<PubSubPublisher> logger, PubSubOptions options, EmulatorDetection emulatorDetection)
    {
        _logger = logger; 
        _publisherClient = new PublisherClientBuilder
        {
            TopicName = TopicName.FromProjectTopic(options.ProjectId, options.TopicId),
            Settings = new PublisherClient.Settings
            {
                EnableMessageOrdering = true
            },
            EmulatorDetection = emulatorDetection,            
        }.Build();
    }

    public async Task PublishAsync(string msg, string messageType, string source)
    {
        var m = new PubsubMessage
        {
            Data = ByteString.CopyFromUtf8(msg),
            Attributes =
        {
            { "MessageType", messageType },
            { "Source", source },
            { "X-Cloud-Trace-Context", XXX }, //TODO: how ???
        }
        };
        var perfTimer = Stopwatch.StartNew();
        await _publisherClient.PublishAsync(m);
        _logger.LogInformation("Published message {msg} in {ellapsedTime}ms", msg, perfTimer.ElapsedMilliseconds);
    }
}

public class PubSubEndpointMiddleware
{
   public async Task InvokeAsync(HttpContext httpContext)
  {
        //TODO: detect it is a PUSH endpoint with [PubSubEndpoint]
        //TODO: extract attribute X-Cloud-Trace-Context from JSON body
        //TODO: populate the current trace : how ????
        await _next(httpContext);        
    }
}
@amanda-tarafa amanda-tarafa self-assigned this Dec 15, 2022
@amanda-tarafa amanda-tarafa added type: question Request for information or clarification. Not an issue. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Dec 15, 2022
@jskeet
Copy link
Collaborator

jskeet commented Dec 15, 2022

We are hoping to invest in telemetry/observability significantly next year, which I hope would accomplish what you need. Assigning to @Rishabh-V who is likely to take this work on; we may end up wanting to close this specific issue (rather than have it open all the time that we're doing the more general work) but we'll see.

@christopheblin
Copy link
Author

@jskeet thanks for the input

I'll try to put something up before next year if I can ...

Do you have an idea for the 2 technical points:

  1. how to extract the current trace context in order to add an attribute in the pubsub message ?
  2. how to populate the current trace context in order for next calls to _logger.LogXxx to be part of it ?

@jskeet
Copy link
Collaborator

jskeet commented Dec 16, 2022

Sorry, I'm not in a position to answer those questions (especially as I'm basically on vacation now) - but I'd urge you not to spend much time writing code for that yourself if we'll be doing a wider design shortly anyway.

@christopheblin
Copy link
Author

I did not understand that it was going to be done soon (when you said next year, in my head, it was like december next year 😄 )

I'll wait then, please keep me in touch when you have an idea about the delay (like if you create a "bigger" issue to track this, please close this one as duplicate)

Have a good vacation and take care of you 🍸

@amanda-tarafa amanda-tarafa removed their assignment Dec 19, 2022
@amanda-tarafa
Copy link
Contributor

I'll close this one as it is contained by #8366 . Please follow that issue for updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: question Request for information or clarification. Not an issue.
Projects
None yet
Development

No branches or pull requests

4 participants