Added OTLP/HTTP JSON Exporter #210

riticksingh · 2020-11-04T09:14:16Z

Signed-off-by:Ritick Gautam[email protected]

linux-foundation-easycla · 2020-11-04T09:14:20Z

The committers are authorized under a signed CLA.

✅ Ritick Gautam (133e86a)

codecov · 2020-11-04T09:15:14Z

Codecov Report

Merging #210 (16e6604) into master (16c3f97) will increase coverage by 0.57%.
The diff coverage is 98.59%.

@@             Coverage Diff              @@
##             master     #210      +/-   ##
============================================
+ Coverage     91.82%   92.40%   +0.57%     
- Complexity      321      353      +32     
============================================
  Files            36       38       +2     
  Lines           758      829      +71     
============================================
+ Hits            696      766      +70     
- Misses           62       63       +1

Impacted Files	Coverage Δ	Complexity Δ
contrib/Otlp/Exporter.php	`97.67% <97.67%> (ø)`	`21.00 <21.00> (?)`
contrib/Otlp/SpanConverter.php	`100.00% <100.00%> (ø)`	`11.00 <11.00> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16c3f97...16e6604. Read the comment docs.

prondubuisi · 2020-11-04T09:44:43Z

Hello @riticksingh did you get an email from EasyCLA confirming your signature signed? If so do confirm that you are using the same email for your Github push. This is what my email looked like

riticksingh · 2020-11-04T10:55:00Z

yes I got the mail from easyCLA on the same email id which I use for github but still it is showing missing ID commit

bobstrecansky · 2020-11-04T13:19:08Z

Looking at your individual commits, it looks like you've used a different email address in your local author's git config. I checked out your branch locally, and I see the author on your last 3 commits being listed as [email protected] rather than [email protected]. I think this is the cause of the CLA check failing. Output below:

$ git log -n 3

commit 740351c8a4aef48bac6eb27251f393198e998aab (HEAD -> ritickmaster, ritick/master)
Author: riticksingh <[email protected]>
Date:   Wed Nov 4 17:11:52 2020 +0530

    Updated OTLPExporterTest

commit e00896e491565f6148871e8aa160036c221dfcef
Author: riticksingh <[email protected]>
Date:   Wed Nov 4 16:45:27 2020 +0530

    Added test case for OTLPExporter

    Signed-off-by:Ritick Gautam

commit be556ad13fc4d9e3191c7cce7b5bdebd7bc7fd61
Author: riticksingh <[email protected]>
Date:   Wed Nov 4 14:40:45 2020 +0530

    Added OTLP/HTTP JSON Exporter

    Signed-off-by:Ritick Gautam<[email protected]>

bobstrecansky · 2020-11-04T13:19:40Z

You can set your author name and email using the following tutorial:
https://www.git-tower.com/learn/git/faq/change-author-name-email/

dkarlovi · 2020-11-04T16:36:55Z

contrib/otlp/Exporter.php

+        $this->certificateFile = getenv("OTEL_EXPORTER_OTLP_CERTIFICATE") ?: "none";
+        $this->headers[] = getenv("OTEL_EXPORTER_OTLP_HEADERS") ?: "none";
+        $this->compression = getenv("OTEL_EXPORTER_OTLP_COMPRESSION") ?: "none";
+        $this->timeout = getenv("OTEL_EXPORTER_OTLP_TIMEOUT") ?: 10;


?: is unsafe here since it will fail with 0.

https://3v4l.org/lmWjD

You're free to change this - it's your PR 👍

Eh, it might be a case of mistaken identity here. :)

Sorry, was referring to @riticksingh - I could see how that would be confusing!

adhocore · 2020-11-05T14:30:16Z

contrib/otlp/Exporter.php

+
+declare(strict_types=1);
+
+namespace OpenTelemetry\Contrib\otlp;


is it typo or really lowercase otlp ?

Other languages also use lower case, but I'm wondering if it's more PHP like to use capitals here. I don't have strong opinions.

imo StuldyCase is preferred more than camelCase or lowercase in class name and in namespace.

StudlyCase would be fine with me too:

namespace OpenTelemetry\Contrib\Otlp;

updated the namespace have a look and thanks for this 👍

Signed-off-by:Ritick Gautam<[email protected]>

Updated TestCase

Updated: testcase

bobstrecansky

one comment on naming but otherwise this looks good to me! Will let other contributors chime in.

bobstrecansky · 2020-11-06T13:25:19Z

contrib/otlp/Exporter.php

+
+declare(strict_types=1);
+
+namespace OpenTelemetry\Contrib\otlp;


Other languages also use lower case, but I'm wondering if it's more PHP like to use capitals here. I don't have strong opinions.

morrisonlevi

Is the formatter running checks on this? The formatting for the constructor looked a bit weird, and there aren't trailing newlines in some of the files (which I thought was part of the checks, but maybe not).

bobstrecansky · 2020-11-08T18:41:17Z

Good catch @morrisonlevi - I ran php-cs-fixer on this using make style and noticed a couple fixes.

@riticksingh - can you perform that action and push it to this branch? I'll open a separate issue to determine why this isn't being applied during CI.

riticksingh requested review from beniamin, bobstrecansky and morrisonlevi as code owners November 4, 2020 09:14

dkarlovi reviewed Nov 4, 2020

View reviewed changes

adhocore reviewed Nov 5, 2020

View reviewed changes

riticksingh added 10 commits November 5, 2020 22:37

Added OTLP/HTTP JSON Exporter

f55e887

Signed-off-by:Ritick Gautam<[email protected]>

Updated SpanConverter

3d76f60

Fix vendor/bin/psalm

e7b3cd9

Updated TestCase

e5bfbfa

Updated OtlpExportertest accordingly #208

542887e

Updated TestCase

Updated OTLPSpanConverterTest

635da25

Updated OTLPExportertest

dedd9f7

BugFixed

a430905

bug fixed

d026aa3

Updated: otlpExporter

5b974a9

Updated: testcase

bobstrecansky approved these changes Nov 6, 2020

View reviewed changes

riticksingh added 2 commits November 7, 2020 11:25

Updated namespace with StuldyCase

b3c3273

Renamed the namespace

bdd4ca3

bobstrecansky approved these changes Nov 7, 2020

View reviewed changes

morrisonlevi reviewed Nov 7, 2020

View reviewed changes

bobstrecansky mentioned this pull request Nov 8, 2020

php-cs-fixer not being applied in CI #212

Closed

Updated php-cs-fixer

16e6604

bobstrecansky merged commit 094333d into open-telemetry:master Nov 10, 2020

bobstrecansky mentioned this pull request Nov 11, 2020

Feature - OpenTelemetry Exporter #130

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added OTLP/HTTP JSON Exporter #210

Added OTLP/HTTP JSON Exporter #210

riticksingh commented Nov 4, 2020

linux-foundation-easycla bot commented Nov 4, 2020 •

edited

Loading

codecov bot commented Nov 4, 2020 •

edited

Loading

prondubuisi commented Nov 4, 2020 •

edited

Loading

riticksingh commented Nov 4, 2020 •

edited

Loading

bobstrecansky commented Nov 4, 2020

bobstrecansky commented Nov 4, 2020

dkarlovi Nov 4, 2020

bobstrecansky Nov 4, 2020

dkarlovi Nov 4, 2020

bobstrecansky Nov 4, 2020

dkarlovi Nov 4, 2020

adhocore Nov 5, 2020

riticksingh Nov 5, 2020

bobstrecansky Nov 6, 2020

adhocore Nov 6, 2020 •

edited

Loading

bobstrecansky Nov 6, 2020

riticksingh Nov 7, 2020

bobstrecansky left a comment

bobstrecansky Nov 6, 2020

morrisonlevi left a comment

bobstrecansky commented Nov 8, 2020


		declare(strict_types=1);

		namespace OpenTelemetry\Contrib\otlp;

Added OTLP/HTTP JSON Exporter #210

Added OTLP/HTTP JSON Exporter #210

Conversation

riticksingh commented Nov 4, 2020

linux-foundation-easycla bot commented Nov 4, 2020 • edited Loading

codecov bot commented Nov 4, 2020 • edited Loading

Codecov Report

prondubuisi commented Nov 4, 2020 • edited Loading

riticksingh commented Nov 4, 2020 • edited Loading

bobstrecansky commented Nov 4, 2020

bobstrecansky commented Nov 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adhocore Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobstrecansky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morrisonlevi left a comment

Choose a reason for hiding this comment

bobstrecansky commented Nov 8, 2020

linux-foundation-easycla bot commented Nov 4, 2020 •

edited

Loading

codecov bot commented Nov 4, 2020 •

edited

Loading

prondubuisi commented Nov 4, 2020 •

edited

Loading

riticksingh commented Nov 4, 2020 •

edited

Loading

adhocore Nov 6, 2020 •

edited

Loading