Skip to content

Switch APM Agent for NodeJS to Asciidoctor #724

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

Merged
merged 1 commit into from
Mar 15, 2019

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Mar 15, 2019

Switches the core of the docs build process from the
no-longer-maintained AsciiDoc to the actively-maintained Asciidoctor.
The resulting HTML is mostly the same but there are a few differences:

  1. Spacing changes that the browser ignores
  2. AsciiDoc adds <p></p> inside any empty table cell. Asciidoctor does
    not.
  3. The automatically generated alt text for one of the images is
    different in Asciidoctor. I believe the Asciidoctor alt text is
    marginally better but I don't think automatically generated alt text is
    a good thing in general.
  4. There is a particular table cell that seems to be changed like so:
-                ^2.0.0 || ^3.1.0
+                \^2.0.0 || ^3.1.0

I think we can probably change the table cell in the docs after merging
Asciidoctor.
5. One of the deprecation warnings looks a little sad:

-              Span started automatically by
-              <a class="link" href="agent-api.html#apm-start-span" title="apm.startSpan([name][, type])">
-               apm.startSpan()
-              </a>
+              Span started automatically by &lt;&lt;apm-start-span

I'm going to investigate this one further before we merge this. I expect
Asciidoctor has a non-greedy regex instead of a greedy one. We might
just have to work around this. This seems to be a rare thing. But I'll
check!

Switches the core of the docs build process from the
no-longer-maintained AsciiDoc to the actively-maintained Asciidoctor.
The resulting HTML is mostly the same but there are a few differences:
1. Spacing changes that the browser ignores
2. AsciiDoc adds `<p></p>` inside any empty table cell. Asciidoctor does
not.
3. The automatically generated alt text for one of the images is
different in Asciidoctor. I believe the Asciidoctor alt text is
marginally better but I don't think automatically generated alt text is
a good thing in general.
4. There is a particular table cell that seems to be changed like so:
-                ^2.0.0 || ^3.1.0
+                \^2.0.0 || ^3.1.0

I think we can probably change the table cell in the docs after merging
Asciidoctor.
5. One of the deprecation warnings looks a little sad:
-              Span started automatically by
-              <a class="link" href="agent-api.html#apm-start-span" title="apm.startSpan([name][, type])">
-               apm.startSpan()
-              </a>
+              Span started automatically by &lt;&lt;apm-start-span

I'm going to investigate this one further before we merge this. I expect
Asciidoctor has a non-greedy regex instead of a greedy one. We might
just have to work around this. This seems to be a rare thing. But I'll
check!
@nik9000 nik9000 requested a review from bmorelli25 March 15, 2019 16:27
@nik9000
Copy link
Member Author

nik9000 commented Mar 15, 2019

@bmorelli25
Copy link
Member

I agree with 4 - that's just a backslash in the asciidoc that shouldn't be there.
I played around with 5, an easy workaround is to just remove the link text:
deprecated[1.1.0,Span started automatically by <<apm-start-span>>]
Which looks like:
Screen Shot 2019-03-15 at 9 42 08 AM

@nik9000
Copy link
Member Author

nik9000 commented Mar 15, 2019

I worked around 5 by doing this:

[manybubbles@localhost apm-agent-nodejs]$ git diff
diff --git a/docs/span-api.asciidoc b/docs/span-api.asciidoc
index df9a4dd..49375e3 100644
--- a/docs/span-api.asciidoc
+++ b/docs/span-api.asciidoc
@@ -54,7 +54,7 @@ the following are standardized across all Elastic APM agents:
 [[span-start]]
 ==== `span.start()`
 
-deprecated[1.1.0,Span started automatically by <<apm-start-span,apm.startSpan()>>]
+deprecated[1.1.0,"Span started automatically by <<apm-start-span,apm.startSpan()>>"]
 
 [source,js]
 ----

@bmorelli25
Copy link
Member

bmorelli25 commented Mar 15, 2019

I'm not sure that's ideal though, the quotation marks do render:
Screen Shot 2019-03-15 at 10 50 41 AM

Edit: Looks like it doesn't have to be quotes. Bold/italics work as well:
deprecated[1.1.0,*Span started automatically by <<apm-start-span,apm.startSpan()>>*]

deprecated[1.1.0,_Span started automatically by <<apm-start-span,apm.startSpan()>>_]

@bmorelli25
Copy link
Member

bmorelli25 commented Mar 15, 2019

Oh here we go. We can just put it into a code snippet:

deprecated[1.1.0,Span started automatically by `<<apm-start-span,apm.startSpan()>>`]

Edit: nope, this doesn't work

Copy link
Member

@bmorelli25 bmorelli25 left a comment

Choose a reason for hiding this comment

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

@nik9000
Copy link
Member Author

nik9000 commented Mar 15, 2019

Oh here we go. We can just put it into a code snippet:

That doesn't render right for me in Asciidoctor. I think we might have hit something that is incompatible between the two. My fix works for Asciidoctor only and looks wrong in Asciidoc. Yours looks wrong in Asciidoctor....

@bmorelli25
Copy link
Member

Good call, I was testing with asciidoc still on accident 🤦‍♂️ . Updated my comments above, and fixed the Node PR to something that works with asciidoctor.

@nik9000 nik9000 merged commit d80c687 into elastic:master Mar 15, 2019
@nik9000 nik9000 mentioned this pull request Mar 15, 2019
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.

2 participants