[POC] Naming cleanup #203

RassK · 2021-07-21T13:10:33Z

Fixes #

Mainly consistent naming all over project.
Inconsistency of build output naming in Windows vs Linux / MacOS (due MSBuild vs Make)

pjanotti

Name changes themselves look good but I think the important thing is to help test and validate by checking out the branch and building it (via VS, CLI, etc). I will be doing a bit of that.

pjanotti · 2021-07-22T17:37:19Z

build/nuke/Build.Steps.Windows.cs

@@ -63,7 +63,7 @@ partial class Build
            foreach (var architecture in ArchitecturesForPlatform)
            {
                var source = NativeProfilerProject.Directory / "bin" / BuildConfiguration / architecture.ToString() /
-                             "OpenTelemetry.AutoInstrumentation.ClrProfiler.Native.dll";
+                             $"{NativeProfilerProject.Name}.dll";


pjanotti · 2021-07-22T18:41:44Z

build/nuke/Build.cs

@@ -17,7 +17,7 @@
    InvokedTargets = new[] { nameof(Workflow) })]
 partial class Build : NukeBuild
 {
-    public static int Main () => Execute<Build>(x => x.BuildTracer);


How did you notice the spacing issue? StyleCop on VS?

VS auto formatting (ctrl + f) is in hand like ctrl + s 😄

also I think it's entry point and I was playing around with that line too

pjanotti · 2021-07-22T18:57:12Z

@RassK I got the following trying to run the ./poc.sh via Git bash on Windows:

Build FAILED.

"C:\otel.net.instr\OpenTelemetry.AutoInstrumentation.proj" (BuildCpp target) (1) ->
"C:\otel.net.instr\src\OpenTelemetry.AutoInstrumentation.ClrProfiler.Native\OpenTelemetry.AutoInstrumentation.ClrProfiler.Native.DLL.vcxproj" (Build target) (2) ->
"C:\otel.net.instr\src\OpenTelemetry.AutoInstrumentation.ClrProfiler.Native\OpenTelemetry.AutoInstrumentation.ClrProfiler.Native.vcxproj" (default target) (3) ->
(Lib target) ->
  LINK : fatal error LNK1181: cannot open input file 'lib\fmt_x64-windows-static\debug\lib\fmtd.lib' [C:\otel.net.instr\src\OpenTelemetry.AutoInstrumentation.ClrProfiler.Native\OpenTelemetry.AutoInstrumentation.ClrProfiler.Native.vcxproj]

    0 Warning(s)
    1 Error(s)

What will be the way to run poc.sh on Windows? It is fine for it to eventually disable that script but then we need to update the docs - that said I would rather have some easy way to build and run an instrumented application, the poc.sh is really convenient.

pjanotti · 2021-07-22T19:09:09Z

After building with nuke on VS dev prompt I am getting modified: .github/workflows/ci.yml it is just the CR/LF:

warning: CRLF will be replaced by LF in .github/workflows/ci.yml.
The file will have its original line endings in your working directory

Since there is a .gitattributes file on the repo it may be good to add a rule for this file to avoid this.

Nowadays, I think everything should simply be LF only since nowadays even notepad understand them.

RassK · 2021-07-23T10:34:06Z

@RassK I got the following trying to run the ./poc.sh via Git bash on Windows:
Build FAILED.

"C:\otel.net.instr\OpenTelemetry.AutoInstrumentation.proj" (BuildCpp target) (1) ->
"C:\otel.net.instr\src\OpenTelemetry.AutoInstrumentation.ClrProfiler.Native\OpenTelemetry.AutoInstrumentation.ClrProfiler.Native.DLL.vcxproj" (Build target) (2) ->
"C:\otel.net.instr\src\OpenTelemetry.AutoInstrumentation.ClrProfiler.Native\OpenTelemetry.AutoInstrumentation.ClrProfiler.Native.vcxproj" (default target) (3) ->
(Lib target) ->
  LINK : fatal error LNK1181: cannot open input file 'lib\fmt_x64-windows-static\debug\lib\fmtd.lib' [C:\otel.net.instr\src\OpenTelemetry.AutoInstrumentation.ClrProfiler.Native\OpenTelemetry.AutoInstrumentation.ClrProfiler.Native.vcxproj]

    0 Warning(s)
    1 Error(s)
What will be the way to run poc.sh on Windows? It is fine for it to eventually disable that script but then we need to update the docs - that said I would rather have some easy way to build and run an instrumented application, the poc.sh is really convenient.

Maybe you are missing clean? poc.sh directs build to build_poc.sh the same one that GH Workflow is using.
Currently I'm running it from VS Code (Bash terminal) but eventually when managed layer is nuked (we should have a task about that) then we can direct all build to nuke, even samples.

RassK · 2021-07-23T11:04:28Z

After building with nuke on VS dev prompt I am getting modified: .github/workflows/ci.yml it is just the CR/LF:
warning: CRLF will be replaced by LF in .github/workflows/ci.yml.
The file will have its original line endings in your working directory
Since there is a .gitattributes file on the repo it may be good to add a rule for this file to avoid this.

Nowadays, I think everything should simply be LF only since nowadays even notepad understand them.

Interesting. Can you try playing around with it, I cannot replicate:
Widows - Visual Studio (Package manager console) - OK
MacOS - Visual Studio for Mac (terminal) - OK
Linux - Visual Studio Code (terminal) - OK

pellared · 2021-07-26T08:36:44Z

After building with nuke on VS dev prompt I am getting modified: .github/workflows/ci.yml it is just the CR/LF:
warning: CRLF will be replaced by LF in .github/workflows/ci.yml.
The file will have its original line endings in your working directory
Since there is a .gitattributes file on the repo it may be good to add a rule for this file to avoid this.
Nowadays, I think everything should simply be LF only since nowadays even notepad understand them.
Interesting. Can you try playing around with it, I cannot replicate:
Widows - Visual Studio (Package manager console) - OK
MacOS - Visual Studio for Mac (terminal) - OK
Linux - Visual Studio Code (terminal) - OK

For me, everything is fine as well. I suggest adding a git diff --exit-code target to Workflow pipeline to check what will happen in GH Actions.

pellared · 2021-07-26T08:42:33Z

I have run a search for "Datadog" in VS Code and found:

missing renames in docs
missing rename in /samples/Samples.AspNetCoreMvc21/Controllers/HomeController.cs:21 (it is probably a bug)
probably DATADOG_ can be removed from /samples/Samples.AspNetCoreMvc21/Controllers/HomeController.cs:25

I think we can add GitHubActionsImage.UbuntuLatest and GitHubActionsImage.MacOsLatest to build/nuke/Build.cs.

pjanotti · 2021-07-27T16:50:25Z

Likely, the diff I'm seeing is because I use core.autocrlf=input

$ git diff --exit-code
warning: CRLF will be replaced by LF in .github/workflows/ci.yml.
The file will have its original line endings in your working directory

# Conflicts: # .github/dependabot.yml # OpenTelemetry.AutoInstrumentation.sln # build/nuke/Build.Steps.Windows.cs # build_poc.sh # dev/envvars.sh # src/OpenTelemetry.AutoInstrumentation.ClrProfiler.Native/CMakeLists.txt

pjanotti

LGTM

pellared

Let's merge it 😉

RassK added 3 commits July 21, 2021 12:59

fix gitignore

7764800

refactor project namings

e34feae

refactor native project dir name

c374d58

RassK requested a review from a team July 21, 2021 13:10

pjanotti reviewed Jul 22, 2021

View reviewed changes

RassK added 4 commits August 10, 2021 12:59

Merge branch 'poc-otel-sdk' into naming-cleanup-otel

9d7eb41

# Conflicts: # .github/dependabot.yml # OpenTelemetry.AutoInstrumentation.sln # build/nuke/Build.Steps.Windows.cs # build_poc.sh # dev/envvars.sh # src/OpenTelemetry.AutoInstrumentation.ClrProfiler.Native/CMakeLists.txt

fix sample

c75330e

clean sln file

08bc421

fix make definititons

3e4dc1c

RassK requested a review from pjanotti August 10, 2021 13:09

RassK added 2 commits August 11, 2021 12:50

remove unnecessary constants

dad2572

Merge branch 'poc-otel-sdk' into naming-cleanup-otel

fec2242

pjanotti approved these changes Aug 11, 2021

View reviewed changes

pellared approved these changes Aug 11, 2021

View reviewed changes

pjanotti merged commit 4e2fd9e into open-telemetry:poc-otel-sdk Aug 11, 2021

RassK deleted the naming-cleanup-otel branch August 12, 2021 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[POC] Naming cleanup #203

[POC] Naming cleanup #203

RassK commented Jul 21, 2021

pjanotti left a comment

pjanotti Jul 22, 2021

pjanotti Jul 22, 2021

RassK Jul 23, 2021

pjanotti commented Jul 22, 2021

pjanotti commented Jul 22, 2021

RassK commented Jul 23, 2021

RassK commented Jul 23, 2021

pellared commented Jul 26, 2021 •

edited

Loading

pellared commented Jul 26, 2021

pjanotti commented Jul 27, 2021

pjanotti left a comment

pellared left a comment •

edited

Loading

[POC] Naming cleanup #203

[POC] Naming cleanup #203

Conversation

RassK commented Jul 21, 2021

pjanotti left a comment

Choose a reason for hiding this comment

pjanotti Jul 22, 2021

Choose a reason for hiding this comment

pjanotti Jul 22, 2021

Choose a reason for hiding this comment

RassK Jul 23, 2021

Choose a reason for hiding this comment

pjanotti commented Jul 22, 2021

pjanotti commented Jul 22, 2021

RassK commented Jul 23, 2021

RassK commented Jul 23, 2021

pellared commented Jul 26, 2021 • edited Loading

pellared commented Jul 26, 2021

pjanotti commented Jul 27, 2021

pjanotti left a comment

Choose a reason for hiding this comment

pellared left a comment • edited Loading

Choose a reason for hiding this comment

pellared commented Jul 26, 2021 •

edited

Loading

pellared left a comment •

edited

Loading