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

OTLP/grpc #230

Closed
wants to merge 15 commits into from
Closed

OTLP/grpc #230

wants to merge 15 commits into from

Conversation

riticksingh
Copy link
Contributor

No description provided.

Copy link
Collaborator

@bobstrecansky bobstrecansky left a comment

Choose a reason for hiding this comment

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

A couple questions to start!

contrib/OtlpGrpc/SpanConverter.php Outdated Show resolved Hide resolved
contrib/OtlpGrpc/Exporter.php Outdated Show resolved Hide resolved
contrib/OtlpGrpc/Exporter.php Outdated Show resolved Hide resolved

list($response, $status) = $client->Export($request)->wait();
if ($status->code !== Grpc\STATUS_OK) {
echo "ERROR: " . $status->code . ", " . $status->details . PHP_EOL;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason this is an error and exit(1) rather than a return value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not any specifc reason for error , just it will get exit if the status is not equal to STATUS_OK

Copy link
Contributor

Choose a reason for hiding this comment

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

But the exit(1) will completely shutdown the execution of the application, where the library is installed. Is this a desired behaviour?

contrib/OtlpGrpc/SpanConverter.php Outdated Show resolved Hide resolved
@bobstrecansky
Copy link
Collaborator

@riticksingh - what else needs to be completed (besides the conflict resolution) for this PR?

@bobstrecansky
Copy link
Collaborator

@jodeev - I believe @riticksingh is still working on this and it's not ready for review yet. I'm going to move it back to in progress.

Base automatically changed from master to main January 22, 2021 17:13
@bobstrecansky
Copy link
Collaborator

@riticksingh - are you still working on this PR?

@SeanHood
Copy link
Contributor

SeanHood commented Mar 2, 2021

Hey folks, I was interested in the PR and have had a play around. I've got a working PoC of otel-php sending spans to the otel-collector and to Honeycomb.io over GRPC.

Certainly not ready for prime time, but I'll get the commits up on a branch shortly.

@bobstrecansky
Copy link
Collaborator

Sounds great, would love to see it @SeanHood !


declare(strict_types=1);
namespace OpenTelemetry\Contrib\OtlpGrpc;
require __DIR__ . '/../../vendor/autoload.php';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to require the autoload here? I think it should stay only in the entry-point of an application.

<?php

declare(strict_types=1);
require __DIR__ . '/../vendor/autoload.php';
Copy link
Contributor

Choose a reason for hiding this comment

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

same case as above.

@SeanHood
Copy link
Contributor

SeanHood commented Mar 4, 2021

As promised: SeanHood@cf54582

The commit has more info of what still needs sorting (that I can see).

Sample from what I get in the otel collector:

2021-03-04T01:11:48.495Z	INFO	loggingexporter/logging_exporter.go:313	TracesExporter	{"#spans": 1}
2021-03-04T01:11:48.495Z	DEBUG	loggingexporter/logging_exporter.go:352	ResourceSpans #0
InstrumentationLibrarySpans #0
InstrumentationLibrary otel-php 0.0.1
Span #0
    Trace ID       : 36643638663665623732656362306563
    Parent ID      :
    ID             : 3432396630626135
    Name           : http_get
    Kind           : SPAN_KIND_INTERNAL
    Start time     : 2021-03-04 01:11:48.210354944 +0000 UTC
    End time       : 2021-03-04 01:11:48.365279044 +0000 UTC
    Status code    : STATUS_CODE_OK
    Status message : Description
Attributes:
     -> request.path: STRING(/)
     -> request.url: STRING(http://0.0.0.0:8000)
     -> request.method: STRING(GET)
     -> request.secure: BOOL(false)
     -> request.ip: STRING(172.19.0.1)
     -> request.ua: STRING(Mozilla/5.0 (Macintosh; Intel Mac OS X 11_0_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/87.0.4280.141 Safari/537.36 OPR/73.0.3856.344)
     -> response.status: INT(200)

@bobstrecansky
Copy link
Collaborator

Going to close this, as @SeanHood 's PR supersedes this one.

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