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

Update installation steps #1933

Merged
merged 2 commits into from
Mar 10, 2022
Merged

Update installation steps #1933

merged 2 commits into from
Mar 10, 2022

Conversation

delner
Copy link
Contributor

@delner delner commented Mar 7, 2022

The GettingStarted.md documentation could be a bit clearer. Specifically we want to add details about UDS configuration and OTLP, but to have sufficient clarity, the install steps require a bit of rework.

This pull request:

  • Changes the Installation section to describe configuring the agent, and configuring the application to connect to it.
  • Condenses much of the Advanced configuration section into a table, eliminates a few sections, updates a few others.

Will probably be easiest to review as a whole document, rather than as lines changed.

@delner delner added docs Involves documentation dev/refactor Involves refactoring existing components labels Mar 7, 2022
@delner delner added this to the 1.0.0.beta2 milestone Mar 7, 2022
@delner delner requested a review from a team March 7, 2022 23:54
@delner delner self-assigned this Mar 7, 2022
@delner delner force-pushed the docs/install_steps_rewrite branch from 4056825 to ef59239 Compare March 8, 2022 00:05
@delner delner added this to Review in progress in 1.0 Mar 8, 2022

### Connect your application to the Datadog Agent

`ddtrace` will submit traces to the agent via UDS by default (if a socket is discovered.) Otherwise, it will fallback and submit traces via HTTP to `127.0.0.1:8126`.

Choose a reason for hiding this comment

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

Suggested change
`ddtrace` will submit traces to the agent via UDS by default (if a socket is discovered.) Otherwise, it will fallback and submit traces via HTTP to `127.0.0.1:8126`.
`ddtrace` will submit traces to the agent via UDS by default (if a socket is discovered). Otherwise, it will fallback and submit traces via HTTP to `127.0.0.1:8126`.

Can you confirm that this case is only when the trace agent target is not set? If it is the case it can be mentioned there

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 sure what you mean by only when the trace agent target is not set?

Logic is simple: it looks for socket file. If it finds it at the default location it uses it. Otherwise it uses configured options. If no options are configured it uses the defaults for those, which is 127.0.0.1:8126.

Copy link
Contributor

Choose a reason for hiding this comment

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

So IIUC it will use an autodetected socket over any user-set configuration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in agent settings resolver:

def adapter
  # If no agent settings have been provided, we try to connect using a local unix socket.
  # We only do so if the socket is present when `ddtrace` runs.
  if should_use_uds_fallback?
    Transport::Ext::UnixSocket::ADAPTER
  else
    Transport::Ext::HTTP::ADAPTER
  end
end

def should_use_uds_fallback?
  uds_fallback != nil
end

# We only use the default unix socket if it is already present.
# This is by design, as we still want to use the default host:port if no unix socket is present.
def uds_fallback
  return @uds_fallback if defined?(@uds_fallback)

  @uds_fallback =
    if configured_hostname.nil? &&
        configured_port.nil? &&
        deprecated_for_removal_transport_configuration_proc.nil? &&
        File.exist?(Transport::Ext::UnixSocket::DEFAULT_PATH)

      Transport::Ext::UnixSocket::DEFAULT_PATH
    end
end

DEFAULT_PATH = '/var/run/datadog/apm.socket'.freeze

So looks like if a hostname/port is explicitly configured, then it will use that over UDS. But if nothing is configured, and it discovers a socket, it will use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the wording on this section to make it clear it uses configured settings first, otherwise falls back to UDS, and then HTTP.

Copy link
Contributor

@lloeki lloeki left a comment

Choose a reason for hiding this comment

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

I like the adjustments.

Made some suggestions and asked a few Qs. Feel free to take'em or leave'em.

docs/GettingStarted.md Outdated Show resolved Hide resolved
docs/GettingStarted.md Outdated Show resolved Hide resolved
docs/GettingStarted.md Outdated Show resolved Hide resolved
docs/GettingStarted.md Show resolved Hide resolved
docs/GettingStarted.md Outdated Show resolved Hide resolved
docs/GettingStarted.md Outdated Show resolved Hide resolved
docs/GettingStarted.md Outdated Show resolved Hide resolved
docs/GettingStarted.md Outdated Show resolved Hide resolved
docs/GettingStarted.md Outdated Show resolved Hide resolved
docs/GettingStarted.md Outdated Show resolved Hide resolved
@delner delner force-pushed the docs/install_steps_rewrite branch 3 times, most recently from 07ebc68 to ce0d317 Compare March 8, 2022 21:53
@delner delner force-pushed the docs/install_steps_rewrite branch from ce0d317 to 8888e04 Compare March 10, 2022 18:08
@delner delner merged commit 9931c5b into master Mar 10, 2022
@delner delner deleted the docs/install_steps_rewrite branch March 10, 2022 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev/refactor Involves refactoring existing components docs Involves documentation
Projects
1.0
Review in progress
Development

Successfully merging this pull request may close these issues.

None yet

3 participants